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

alion02 · 2024-05-05T04:37:04Z

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 · 2024-05-05T04:37:12Z

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:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

alion02 · 2024-05-05T04:37:31Z

@bors try @rust-timer queue

bors · 2024-05-05T04:37:53Z

@alion02: 🔑 Insufficient privileges: not in try users

Noratrieb · 2024-05-05T05:06:22Z

library/core/src/option.rs

@@ -1037,7 +1037,7 @@ impl<T> Option<T> {
        match self {
            Some(val) => val,
            // SAFETY: the safety contract must be upheld by the caller.
-            None => unsafe { hint::unreachable_unchecked() },
+            None => unsafe { mem::MaybeUninit::uninit().assume_init() },


is this really better? that's disappointing.. how about using offset_of and accessing the value with directly with a pointer offset, entirely removing the condition?

I don't know if it's better in general - I just know it's better in at least one case. I think it's worth running a benchmark for it at least though. (which I can't do 🥺)

TIL about offset_of. uninit seems to be chucked pretty readily, but offset_of might be better in low opt level builds etc.

@Nilstrieb It doesn't seem to have the desired effect:
https://godbolt.org/z/z5fv4as4s

@alion02 Please make your new libcore compile.

workingjubilee · 2024-05-05T05:49:28Z

Might as well try both suggested new impls of unwrap_unchecked.

@bors try @rust-timer queue

bors · 2024-05-05T05:50:38Z

⌛ Trying commit 3d67c90 with merge b25d710...

[Experiment] Replace `unreachable_unchecked()` with `uninit().assume_init()` in `unwrap_unchecked()` [Relevant ACP.](rust-lang/libs-team#378) [Relevant godbolt.](https://godbolt.org/z/WqfcT7Ys9) 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).

bors · 2024-05-05T05:57:05Z

💔 Test failed - checks-actions

workingjubilee · 2024-05-05T05:59:19Z

@bors r-

library/core/src/result.rs

library/core/src/option.rs

workingjubilee · 2024-05-05T06:58:09Z

@bors try

[Experiment] Replace `unreachable_unchecked()` with `uninit().assume_init()` in `unwrap_unchecked()` [Relevant ACP.](rust-lang/libs-team#378) [Relevant godbolt.](https://godbolt.org/z/WqfcT7Ys9) 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).

bors · 2024-05-05T06:59:19Z

⌛ Trying commit fd98988 with merge f471a93...

rust-log-analyzer · 2024-05-05T06:59:31Z

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)

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 · 2024-05-05T08:36:10Z

☀️ Try build successful - checks-actions
Build commit: f471a93 (f471a93d679c528e1a81f68cfad53044030a8d6a)

the8472 · 2024-05-05T13:51:24Z

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 · 2024-05-06T02:08:46Z

@bors try

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

bors · 2024-05-06T02:09:55Z

⌛ Trying commit fd98988 with merge 4a9bbb3...

[Experiment] Replace `unreachable_unchecked()` with `uninit().assume_init()` in `unwrap_unchecked()` [Relevant ACP.](rust-lang/libs-team#378) [Relevant godbolt.](https://godbolt.org/z/WqfcT7Ys9) 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).

Mark-Simulacrum · 2024-05-06T02:11:13Z

@rust-timer queue

bors · 2024-05-06T03:46:56Z

☀️ Try build successful - checks-actions
Build commit: 4a9bbb3 (4a9bbb36013e82d74e72489eb03bf87c7730e48c)

rust-timer · 2024-05-06T12:33:29Z

Finished benchmarking commit (4a9bbb3): 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 · 2024-05-06T17:04:28Z

nom-json got a lot faster (unless it's spurious...?)
two binary size improvements for opt build
a bunch of size regressions for debug

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 · 2024-05-07T22:44:01Z

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

the8472 · 2024-05-07T22:50:20Z

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

Noratrieb · 2024-05-15T14:41:23Z

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.

Noratrieb · 2024-06-04T19:12:19Z

@rustbot author

JohnCSimon · 2024-09-04T02:15:34Z

@alion02
ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Or if you're not going to continue, please close it. Thank you!

alion02 · 2024-09-04T02:19:44Z

Pursuing other priorities. May return another time. Thanks for the reminder.

Replace unreachable_unchecked with uninit().assume_init()

3d67c90

rustbot assigned Noratrieb May 5, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 5, 2024