-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't re-assume
in transmute
s that don't change niches
#137513
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Don't re-`assume` in `transmute`s that don't change niches I noticed in nightly 2025-02-21 that `transmute` is emitting way more `assume`s than necessary for newtypes. For example, the three transmutes in <https://rust.godbolt.org/z/fW1KaTc4o> emits ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: %0 = sub i32 %_1, 1 %1 = icmp ule i32 %0, -2 call void `@llvm.assume(i1` %1) %2 = sub i32 %_1, 1 %3 = icmp ule i32 %2, -2 call void `@llvm.assume(i1` %3) %4 = sub i32 %_1, 1 %5 = icmp ule i32 %4, -2 call void `@llvm.assume(i1` %5) %6 = sub i32 %_1, 1 %7 = icmp ule i32 %6, -2 call void `@llvm.assume(i1` %7) %8 = sub i32 %_1, 1 %9 = icmp ule i32 %8, -2 call void `@llvm.assume(i1` %9) %10 = sub i32 %_1, 1 %11 = icmp ule i32 %10, -2 call void `@llvm.assume(i1` %11) ret i32 %_1 } ``` But those are all just newtypes that don't change size or niches, so none of it's needed. After this PR it's down to just ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: ret i32 %_1 } ``` because none of those `assume`s in the original actually did anything. (Transmuting to something with a difference niche, though, still has the assumes -- the other tests continue to pass checking that.)
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
r? compiler |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (8a03975): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary 2.5%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 770.178s -> 768.923s (-0.16%) |
r? @saethlin |
Love to see us emitting less assumes but not regressing codegen. @bors r+ |
Don't re-`assume` in `transmute`s that don't change niches I noticed in nightly 2025-02-21 that `transmute` is emitting way more `assume`s than necessary for newtypes. For example, the three transmutes in <https://rust.godbolt.org/z/fW1KaTc4o> emits ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: %0 = sub i32 %_1, 1 %1 = icmp ule i32 %0, -2 call void `@llvm.assume(i1` %1) %2 = sub i32 %_1, 1 %3 = icmp ule i32 %2, -2 call void `@llvm.assume(i1` %3) %4 = sub i32 %_1, 1 %5 = icmp ule i32 %4, -2 call void `@llvm.assume(i1` %5) %6 = sub i32 %_1, 1 %7 = icmp ule i32 %6, -2 call void `@llvm.assume(i1` %7) %8 = sub i32 %_1, 1 %9 = icmp ule i32 %8, -2 call void `@llvm.assume(i1` %9) %10 = sub i32 %_1, 1 %11 = icmp ule i32 %10, -2 call void `@llvm.assume(i1` %11) ret i32 %_1 } ``` But those are all just newtypes that don't change size or niches, so none of it's needed. After this PR it's down to just ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: ret i32 %_1 } ``` because none of those `assume`s in the original actually did anything. (Transmuting to something with a difference niche, though, still has the assumes -- the other tests continue to pass checking that.)
💔 Test failed - checks-actions |
@bors retry |
…thlin Don't re-`assume` in `transmute`s that don't change niches I noticed in nightly 2025-02-21 that `transmute` is emitting way more `assume`s than necessary for newtypes. For example, the three transmutes in <https://rust.godbolt.org/z/fW1KaTc4o> emits ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: %0 = sub i32 %_1, 1 %1 = icmp ule i32 %0, -2 call void `@llvm.assume(i1` %1) %2 = sub i32 %_1, 1 %3 = icmp ule i32 %2, -2 call void `@llvm.assume(i1` %3) %4 = sub i32 %_1, 1 %5 = icmp ule i32 %4, -2 call void `@llvm.assume(i1` %5) %6 = sub i32 %_1, 1 %7 = icmp ule i32 %6, -2 call void `@llvm.assume(i1` %7) %8 = sub i32 %_1, 1 %9 = icmp ule i32 %8, -2 call void `@llvm.assume(i1` %9) %10 = sub i32 %_1, 1 %11 = icmp ule i32 %10, -2 call void `@llvm.assume(i1` %11) ret i32 %_1 } ``` But those are all just newtypes that don't change size or niches, so none of it's needed. After this PR it's down to just ```rust define noundef range(i32 1, 0) i32 `@repeatedly_transparent_transmute(i32` noundef range(i32 1, 0) %_1) unnamed_addr { start: ret i32 %_1 } ``` because none of those `assume`s in the original actually did anything. (Transmuting to something with a difference niche, though, still has the assumes -- the other tests continue to pass checking that.)
☀️ Test successful - checks-actions |
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
Finished benchmarking commit (446649d): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -2.0%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 765.603s -> 764.414s (-0.16%) |
I noticed in nightly 2025-02-21 that
transmute
is emitting way moreassume
s than necessary for newtypes.For example, the three transmutes in https://rust.godbolt.org/z/fW1KaTc4o emits
But those are all just newtypes that don't change size or niches, so none of it's needed.
After this PR it's down to just
because none of those
assume
s in the original actually did anything.(Transmuting to something with a difference niche, though, still has the assumes -- the other tests continue to pass checking that.)