-
Couldn't load subscription status.
- Fork 13.9k
Fix MaybeUninit codegen using GVN #147827
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
base: master
Are you sure you want to change the base?
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fix MaybeUninit codegen using GVN
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e6fd12c): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.9%, secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 475.372s -> 474.527s (-0.18%) |
6d353c3 to
1a410f7
Compare
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_codegen_ssa |
|
rustbot has assigned @petrochenkov. Use |
|
r? scottmcm |
This comment has been minimized.
This comment has been minimized.
1a410f7 to
177e9fc
Compare
If you go in that direction, please make sure #137936 is fixed first. Otherwise such a transformation would expose the issue in Rust, as opposed to being merely limited to a custom MIR (probably?). |
This is an alternative to #142837, based on #146355 (comment).
The general approach I took here is to aggressively propagate anything that is entirely uninitialized. GVN generally takes the approach of only synthesizing small types, but we need to generate large consts to fix the codegen issue.
I also added a special case to MIR dumps for this where now an entirely uninit const is printed as
const <uninit>, because otherwise we end up with extremely verbose dumps of the new consts.After GVN though, we still end up with a lot of MIR that looks like this:
Which will break tests/codegen-llvm/maybeuninit-rvo.rs with the naive lowering. I think the ideal fix here is to somehow omit these
_1 = const <uninit>assignments that come directly after a StorageLive, but I'm not sure how to do that. For now at least, ignoring such assignments (even if they don't come right after a StorageLive) in codegen seems to work.Note that since GVN is based on synthesizing a
ConstValuewhich has a defined layout, this scenario still gets deoptimized by LLVM.This case can be handled correctly if enough inlining has happened, or it could be handled by post-mono GVN. Synthesizing
UnevaluatedConstor some other special kind of const seems dubious.