rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
93.58k stars 12.05k forks source link

[Experiment] Replace `unreachable_unchecked()` with `uninit().assume_init()` in `unwrap_unchecked()` #124737

Open alion02 opened 1 week ago

alion02 commented 1 week ago

Relevant ACP. Relevant godbolt.

Attempt to clean up the LLVM IR we generate based on the assumption that we usually don't want to assume the value of the discriminant (because it sticks around in the IR and randomly inhibits optimizations).

rustbot commented 1 week ago

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nilstrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

alion02 commented 1 week ago

@bors try @rust-timer queue

rust-timer commented 1 week ago

Insufficient permissions to issue commands to rust-timer.

bors commented 1 week ago

@alion02: :key: Insufficient privileges: not in try users

rust-log-analyzer commented 1 week ago

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

Click to see the possible cause of the failure (guessed by this bot) ```plain downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/654afe3b540cb1840e9331bd5540c807e7e1484d/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz curl: (22) The requested URL returned error: 404 ERROR: failed to download llvm from ci HELP: There could be two reasons behind this: 1) The host triple is not supported for `download-ci-llvm`. 2) Old builds get deleted after a certain time. HELP: In either case, disable `download-ci-llvm` in your config.toml: [llvm] download-ci-llvm = false Build completed unsuccessfully in 0:00:00 ```
workingjubilee commented 1 week ago

Might as well try both suggested new impls of unwrap_unchecked.

@bors try @rust-timer queue

rust-timer commented 1 week ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 1 week ago

:hourglass: Trying commit 3d67c904ddf9d60a64926ea22dde26f026b37444 with merge b25d7106c60170d8c49899e6980085f954e38542...

rust-log-analyzer commented 1 week ago

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

Click to see the possible cause of the failure (guessed by this bot) ```plain [RUSTC-TIMING] build_script_build test:false 0.207 error[E0433]: failed to resolve: use of undeclared crate or module `mem` --> library/core/src/result.rs:1463:32 | 1463 | Err(_) => unsafe { mem::MaybeUninit::uninit().assume_init() }, | help: consider importing this union through its public re-export | 491 + use crate::mem::MaybeUninit; 491 + use crate::mem::MaybeUninit; | help: if you import `MaybeUninit`, refer to it directly | 1463 - Err(_) => unsafe { mem::MaybeUninit::uninit().assume_init() }, 1463 + Err(_) => unsafe { MaybeUninit::uninit().assume_init() }, error[E0433]: failed to resolve: use of undeclared crate or module `mem` --> library/core/src/result.rs:1494:31 | | 1494 | Ok(_) => unsafe { mem::MaybeUninit::uninit().assume_init() }, | help: consider importing this union through its public re-export | 491 + use crate::mem::MaybeUninit; 491 + use crate::mem::MaybeUninit; | help: if you import `MaybeUninit`, refer to it directly | 1494 - Ok(_) => unsafe { mem::MaybeUninit::uninit().assume_init() }, 1494 + Ok(_) => unsafe { MaybeUninit::uninit().assume_init() }, error: unused import: `hint` --> library/core/src/option.rs:560:19 | --- Caused by: Command RUST_BACKTRACE=full python3 /checkout/x.py build --target x86_64-unknown-linux-gnu --host x86_64-unknown-linux-gnu --stage 2 library/std --rust-profile-generate /tmp/tmp-multistage/opt-artifacts/rustc-pgo --set llvm.thin-lto=false --set llvm.link-shared=true [at /checkout/obj] has failed with exit code Some(1) Stack backtrace: 0: ::msg:: at /rust/deps/anyhow-1.0.81/src/backtrace.rs:27:14 1: ::run at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/exec.rs:78:17 2: ::run at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/exec.rs:179:9 at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:210:13 at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:210:13 4: ::section:: at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/timer.rs:111:22 at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:199:9 at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:199:9 6: ::section:: at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/timer.rs:111:22 at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:196:29 8: opt_dist::main at /rustc/b25d7106c60170d8c49899e6980085f954e38542/src/tools/opt-dist/src/main.rs:386:18 9: core::result::Result<(), anyhow::Error> as core::ops::function::FnOnce<()>>::call_once 9: core::result::Result<(), anyhow::Error> as core::ops::function::FnOnce<()>>::call_once at /rustc/6b544f5ff8d45221d61962651a5f5ab9fe535e16/library/core/src/ops/function.rs:250:5 10: std::sys_common::backtrace::__rust_begin_short_backtrace:: core::result::Result<(), anyhow::Error>, core::result::Result<(), anyhow::Error>> at /rustc/6b544f5ff8d45221d61962651a5f5ab9fe535e16/library/std/src/sys_common/backtrace.rs:155:18 11: std::rt::lang_start::>::{closure#0} at /rustc/6b544f5ff8d45221d61962651a5f5ab9fe535e16/library/std/src/rt.rs:159:18 12: core::ops::function::impls:: for &F>::call_once 13: std::panicking::try::do_call at /rustc/6b544f5ff8d45221d61962651a5f5ab9fe535e16/library/std/src/panicking.rs:559:40 14: std::panicking::try at /rustc/6b544f5ff8d45221d61962651a5f5ab9fe535e16/library/std/src/panicking.rs:523:19 ```
bors commented 1 week ago

:broken_heart: Test failed - checks-actions

workingjubilee commented 1 week ago

@bors r-

rust-log-analyzer commented 1 week ago

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

Click to see the possible cause of the failure (guessed by this bot) ```plain downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/69ffc0d3a3c619009bcb27b8f61d810e27b12612/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz curl: (22) The requested URL returned error: 404 ERROR: failed to download llvm from ci HELP: There could be two reasons behind this: 1) The host triple is not supported for `download-ci-llvm`. 2) Old builds get deleted after a certain time. HELP: In either case, disable `download-ci-llvm` in your config.toml: [llvm] download-ci-llvm = false Build completed unsuccessfully in 0:00:00 ```
rust-log-analyzer commented 1 week ago

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

Click to see the possible cause of the failure (guessed by this bot) ```plain #16 exporting to docker image format #16 sending tarball 29.5s done #16 DONE 35.8s ##[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-17] --- 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-17', '--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', '--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-17/bin/llvm-config configure: llvm.link-shared := True configure: rust.thin-lto-import-instr-limit := 10 configure: change-id := 99999999 ```
workingjubilee commented 1 week ago

@bors try

bors commented 1 week ago

:hourglass: Trying commit fd9898875659a115ff8ea3618e5994e91706ff72 with merge f471a93d679c528e1a81f68cfad53044030a8d6a...

rust-log-analyzer commented 1 week ago

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

Click to see the possible cause of the failure (guessed by this bot) ```plain downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/69ffc0d3a3c619009bcb27b8f61d810e27b12612/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz curl: (22) The requested URL returned error: 404 ERROR: failed to download llvm from ci HELP: There could be two reasons behind this: 1) The host triple is not supported for `download-ci-llvm`. 2) Old builds get deleted after a certain time. HELP: In either case, disable `download-ci-llvm` in your config.toml: [llvm] download-ci-llvm = false Build completed unsuccessfully in 0:00:00 ```
bors commented 1 week ago

:sunny: Try build successful - checks-actions Build commit: f471a93d679c528e1a81f68cfad53044030a8d6a (f471a93d679c528e1a81f68cfad53044030a8d6a)

rust-timer commented 1 week ago

Queued f471a93d679c528e1a81f68cfad53044030a8d6a with parent 69ffc0d3a3c619009bcb27b8f61d810e27b12612, future comparison URL. There are currently 10 preceding artifacts in the queue. It will probably take at least ~13.2 hours until the benchmark run finishes.

the8472 commented 1 week ago

When I tried #102151 it only lead to regressions. But a lot has changed since then and this isn't the same approach so maybe the results will be different this time.

Mark-Simulacrum commented 1 week ago

@bors try

Generate a new try build, the old one is based on a broken master commit AFAICT.

bors commented 1 week ago

:hourglass: Trying commit fd9898875659a115ff8ea3618e5994e91706ff72 with merge 4a9bbb36013e82d74e72489eb03bf87c7730e48c...

Mark-Simulacrum commented 1 week ago

@rust-timer queue

rust-timer commented 1 week ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 1 week ago

:sunny: Try build successful - checks-actions Build commit: 4a9bbb36013e82d74e72489eb03bf87c7730e48c (4a9bbb36013e82d74e72489eb03bf87c7730e48c)

rust-timer commented 1 week ago

Queued 4a9bbb36013e82d74e72489eb03bf87c7730e48c with parent 3170bd9d1bab03eeb15552686f2de84e75360e56, future comparison URL. There are currently 6 preceding artifacts in the queue. It will probably take at least ~8.1 hours until the benchmark run finishes.

rust-timer commented 1 week ago

Finished benchmarking commit (4a9bbb36013e82d74e72489eb03bf87c7730e48c): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never @rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. | | mean | range | count | |:----------------------------------:|:-----:|:--------------:|:-----:| | Regressions ❌
(primary) | 2.8% | [2.8%, 2.8%] | 1 | | Regressions ❌
(secondary) | 5.8% | [5.8%, 5.8%] | 1 | | Improvements ✅
(primary) | -5.0% | [-7.2%, -3.7%] | 3 | | Improvements ✅
(secondary) | - | - | 0 | | All ❌✅ (primary) | -3.1% | [-7.2%, 2.8%] | 4 |

Cycles

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

Binary size

Results This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. | | mean | range | count | |:----------------------------------:|:-----:|:--------------:|:-----:| | Regressions ❌
(primary) | 0.1% | [0.0%, 0.4%] | 33 | | Regressions ❌
(secondary) | - | - | 0 | | Improvements ✅
(primary) | -0.3% | [-0.5%, -0.1%] | 6 | | Improvements ✅
(secondary) | - | - | 0 | | All ❌✅ (primary) | 0.0% | [-0.5%, 0.4%] | 39 |

Bootstrap: 675.885s -> 674.118s (-0.26%) Artifact size: 315.88 MiB -> 315.83 MiB (-0.02%)

alion02 commented 1 week ago

Much smaller impact overall than I expected, but still vaguely interesting.

By the way, I noticed that the x86 backend (which is presumably what these benchmarks are running on) is more resilient to assume spam than ARM. Probably something that can/should be fixed on the LLVM side, but for now — are we able to run a benchmark on ARM?

workingjubilee commented 1 week ago

We don't have the tools/facilities set up for benchmarking Arm, no.

the8472 commented 1 week ago

I assume running rustc-perf locally should work on an arm linux box.

Nilstrieb commented 3 days ago

any improvements from this PR are LLVM bugs, but evidently the LLVM bugs exist. you should report them upstream, but I'd be ok with a bit of papering over them in the meantime. i would prefer the offset_of based approach over this.