assume value ranges in transmute
#109993
assume value ranges in transmute
#109993
Conversation
|
r? @lcnr (rustbot has picked a reviewer for you, use r? to override) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
⌛ Trying commit c7d3c9f10a4c1b95a290936fc1acd585e42f1f5e with merge b359f2b4f1c26bbaf475b4f8cdaa87a57a4f3d82... |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (b359f2b4f1c26bbaf475b4f8cdaa87a57a4f3d82): comparison URL. Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
|
For |
|
r? @oli-obk |
|
@oli-obk I think it depends if it's supposed to work on generics or not. If something like #106281 (comment) happens, then at MIR level we wouldn't reliably know what the type actually is to add these -- MIR could just see type parameters. |
|
Duh... yea, the enum to int casts are not possible on generics, so we never had an issue with that there. |
c7d3c9f to
e11d98b
Compare
e11d98b to
1bcb0ec
Compare
197ccc2 to
baf98e7
Compare
|
Finally got back to this. I've added a bunch of codegen tests to demonstrate that this works as expected, as well as some clarification comments. @rustbot ready |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (7e23d18): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
Stop turning transmutes into discriminant reads in mir-opt Partially reverts rust-lang#109612, as after rust-lang#109993 these aren't actually equivalent any more, and I'm no longer confident this was ever an improvement in the first place. Having this "simplification" meant that similar-looking code actually did somewhat different things. For example, ```rust pub unsafe fn demo1(x: std::cmp::Ordering) -> u8 { std::mem::transmute(x) } pub unsafe fn demo2(x: std::cmp::Ordering) -> i8 { std::mem::transmute(x) } ``` in nightly today is generating <https://rust.godbolt.org/z/dPK58zW18> ```llvm define noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef %x) unnamed_addr #0 { %0 = icmp uge i8 %x, -1 %1 = icmp ule i8 %x, 1 %2 = or i1 %0, %1 call void `@llvm.assume(i1` %2) ret i8 %x } define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef %0) unnamed_addr #0 { %x = alloca i8, align 1 store i8 %0, ptr %x, align 1 %1 = load i8, ptr %x, align 1, !range !2, !noundef !3 ret i8 %1 } ``` Which feels too different when the original code is essentially identical. --- Aside: that example is different *after* optimizations too: ```llvm define noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef returned %x) unnamed_addr #0 { %0 = add i8 %x, 1 %1 = icmp ult i8 %0, 3 tail call void `@llvm.assume(i1` %1) ret i8 %x } define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef returned %0) unnamed_addr #1 { ret i8 %0 } ``` so turning the `Transmute` into a `Discriminant` was arguably just making things worse, so leaving it alone instead -- and thus having less code in rustc -- seems clearly better.
Update our range `assume`s to the format that LLVM prefers I found out in llvm/llvm-project#123278 (comment) that the way I started emitting the `assume`s in rust-lang#109993 was suboptimal, and as seen in that LLVM issue the way we're doing it -- with two `assume`s sometimes -- can at times lead to CVP/SCCP not realize what's happening because one of them turns into a `ne` instead of conveying a range. So this updates how it's emitted from ``` assume( x >= LOW ); assume( x <= HIGH ); ``` to ``` assume( (x - LOW) <= (HIGH - LOW) ); ``` so that we don't need multiple `icmp`s nor multiple `assume`s for a single value.
Update our range `assume`s to the format that LLVM prefers I found out in llvm/llvm-project#123278 (comment) that the way I started emitting the `assume`s in rust-lang#109993 was suboptimal, and as seen in that LLVM issue the way we're doing it -- with two `assume`s sometimes -- can at times lead to CVP/SCCP not realize what's happening because one of them turns into a `ne` instead of conveying a range. So this updates how it's emitted from ``` assume( x >= LOW ); assume( x <= HIGH ); ``` or ``` // (for ranges that wrap the range) assume( (x <= LOW) | (x >= HIGH) ); ``` to ``` assume( (x - LOW) <= (HIGH - LOW) ); ``` so that we don't need multiple `icmp`s nor multiple `assume`s for a single value, and both wrappping and non-wrapping ranges emit the same shape. (And we don't bother emitting the subtraction if `LOW` is zero, since that's trivial for us to check too.)
Fixes #109958