- Notifications
You must be signed in to change notification settings - Fork13.9k
Simplify transmutes in MIR InstCombine#109612
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
These commits modify the If this was intentional then you can ignore this comment. Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
One actionable comment, rest lgtm |
169dfcc todf0e875Compare☔ The latest upstream changes (presumably#106428) made this pull request unmergeable. Pleaseresolve the merge conflicts. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c7ec176 to36d3ea4Compare36d3ea4 to69ca902Compare@bors r+ rollup=iffy |
📌 Commit 69ca902a689f775ca1382738158944d7c4fa259f has been approved by It is now in thequeue for this repository. |
⌛ Testing commit 69ca902a689f775ca1382738158944d7c4fa259f with merge db6c420d803168d261a0e766671cbce72d98328c... |
💔 Test failed -checks-actions |
@bors retry ("Connection reset by peer (os error 104)" in test infra) |
This comment has been minimized.
This comment has been minimized.
⌛ Testing commit 69ca902a689f775ca1382738158944d7c4fa259f with merge f96b6ad273fdc9ef73e191b65d127d9a7546a0d1... |
💔 Test failed -checks-actions |
This comment has been minimized.
This comment has been minimized.
@bors r- (was inadvertently re-queued by a homu sync) |
Thanks to the combination ofrust-lang#108246 andrust-lang#108442 it could already remove identity transmutes.With this PR, it can also simplify them to `IntToInt` casts, `Discriminant` reads, or `Field` projections.
69ca902 tof20af8dComparescottmcm commentedMar 29, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Rebased and made the test a @bors r=cjgillot |
☀️ Test successful -checks-actions |
Finished benchmarking commit (acd27bb):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)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-optPartially revertsrust-lang#109612, as afterrust-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,```rustpub 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>```llvmdefine 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:```llvmdefine 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.
Thanks to the combination of#108246 and#108442 it could already remove identity transmutes.
With this PR, it can also simplify them to
IntToIntcasts,Discriminantreads, orFieldprojections.