rust icon indicating copy to clipboard operation
rust copied to clipboard

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary

Open saethlin opened this issue 1 year ago • 9 comments

This special case was added in this PR: https://github.com/rust-lang/rust/pull/77611 in response to this error message:

Intrinsic has incorrect argument type!
void ({}*)* @llvm.ppc.cfence.p0sl_s
in function rust_oom
LLVM ERROR: Broken function found, compilation aborted!
[RUSTC-TIMING] std test:false 20.161
error: could not compile `std`

But when I tried searching for more information about that intrinsic I found this: https://github.com/llvm/llvm-project/issues/55983 which is a report of someone hitting this same error and a fix was landed in LLVM, 2 years after the above Rust PR.

saethlin avatar Mar 09 '24 02:03 saethlin

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_4323b0ab-a946-424f-ad6b-9a4a7b4c539f
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=ppc-can-into-atomicptr
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_4323b0ab-a946-424f-ad6b-9a4a7b4c539f
GITHUB_REF=refs/pull/122220/merge
GITHUB_REF_NAME=122220/merge
GITHUB_REF_PROTECTED=false
---
#12 writing image sha256:c68491e8929ad297e47a736352c81dbbdf8c28f75989d952cadc7f13648b11e4 done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sat Mar  9 02:55:38 UTC 2024
  network time: Sat, 09 Mar 2024 02:55:38 GMT
  network time: Sat, 09 Mar 2024 02:55:38 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

error: variable does not need to be mutable
   --> compiler/rustc_codegen_ssa/src/mir/intrinsic.rs:354:33
    |
354 | ...                   let mut src = args[2].immediate();
    |                           |
    |                           help: remove this `mut`

error: variable does not need to be mutable

rust-log-analyzer avatar Mar 09 '24 02:03 rust-log-analyzer

The job dist-powerpc64le-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_c022d442-a0f5-4d28-a9e4-269ac5446890
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=ppc-can-into-atomicptr
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_c022d442-a0f5-4d28-a9e4-269ac5446890
GITHUB_REF=refs/pull/122220/merge
GITHUB_REF_NAME=122220/merge
GITHUB_REF_PROTECTED=false
---
  DOC_ARTIFACT_NAME: doc-122220-794d6041
  AWS_ACCESS_KEY_ID: 
  AWS_SECRET_ACCESS_KEY: 
##[endgroup]
cp: cannot stat 'obj/build/metrics.json': No such file or directory
##[error]Process completed with exit code 1.

rust-log-analyzer avatar Mar 09 '24 04:03 rust-log-analyzer

Seems to work?

saethlin avatar Mar 09 '24 15:03 saethlin

@bors rollup=iffy r? oli-obk @antoyo I sank some logic from cg_ssa into cg_llvm, so I feel like some change should be made to cg_gcc? Though I don't know to what degree cg_gcc tests AtomicPtr so I don't know if the tests will pass even if this breaks cg_gcc.

saethlin avatar Mar 09 '24 15:03 saethlin

I do not know for sure if GCC has the same limitations. Does your changes require any new tests?

If not, I can just open an issue on the rustc_codegen_gcc repo and link this PR so I could make the change myself if needed.

antoyo avatar Mar 09 '24 15:03 antoyo

Does your changes require any new tests?

I'm not sure. AtomicPtr already has some good tests in the library, and I confirmed by trial-and-error that if codegen emits invalid calls to the LLVM atomicrmw intrinsics, they are caught by LLVM assertions. I'm not sure how I'd write a test, since LLVM won't even emit IR if I do the wrong thing.

saethlin avatar Mar 09 '24 15:03 saethlin

@bors r+

oli-obk avatar Mar 11 '24 09:03 oli-obk

:pushpin: Commit aa6cfb2669e51b3211017bde2faa6eddba22ecbe has been approved by oli-obk

It is now in the queue for this repository.

bors avatar Mar 11 '24 09:03 bors

:hourglass: Testing commit aa6cfb2669e51b3211017bde2faa6eddba22ecbe with merge e61dcc7a0ac33ef054d76453307124233edcf545...

bors avatar Mar 13 '24 00:03 bors

:sunny: Test successful - checks-actions Approved by: oli-obk Pushing e61dcc7a0ac33ef054d76453307124233edcf545 to master...

bors avatar Mar 13 '24 02:03 bors

Finished benchmarking commit (e61dcc7a0ac33ef054d76453307124233edcf545): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-1.3%, -1.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.147s -> 673.298s (0.17%) Artifact size: 310.26 MiB -> 310.29 MiB (0.01%)

rust-timer avatar Mar 13 '24 05:03 rust-timer