codegen: skip stores for entirely-uninit constant aggregate fields#157797
Conversation
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Uhh...not code I'm familiar with. Let's see if a reroll helps. @rustbot reroll |
|
@rustbot reroll |
|
@rustbot reroll |
|
Reminder, once the PR becomes ready for a review, use |
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (65ff13c): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf 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 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 7.1%, secondary 1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 487.476s -> 487.289s (-0.04%) |
|
@rustbot label: +perf-regression-triaged Perf looks clean and slightly improving things across the board. There is one regression (building cranelift with opts) that has a 16% compile-time perf hit entirely in LLVM, so this change tips the optimizations over the edge somewhere and causes lots of changes. I have not checked whether it produces better runtime code, but let's land it regardless cc @bjorn3 |
|
@bors p=6 |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6f72b5d (parent) -> e7795af (this PR) Test differencesShow 4 test diffsStage 1
Stage 2
Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard e7795af6d2449fb05a6393c3320ced873a999eb3 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (e7795af): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression 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 5.5%, secondary 5.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 12.5%, secondary -10.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 486.811s -> 486.654s (-0.03%) |
| for arg in args { | ||
| match arg.val { | ||
| OperandValue::ZeroSized => {} | ||
| OperandValue::Uninit => {} |
There was a problem hiding this comment.
Skipping uninit arguments looks wrong (similar for gcc implementation):
#![feature(link_llvm_intrinsics)]
use std::mem::*;
extern "C" {
#[link_name = "llvm.sqrt.f32"]
fn sqrt(x: f32) -> f32;
}
fn main() {
std::hint::black_box(unsafe { sqrt(MaybeUninit::uninit().assume_init()) });
}$ env RUSTC_BOOTSTRAP=1 rustc +beta -O i.rs
$ env RUSTC_BOOTSTRAP=1 rustc +nightly -O i.rs
error: failed to parse bitcode for LTO module: Invalid record (Producer: 'LLVM22.1.8-rust-1.99.0-nightly' Reader: 'LLVM 22.1.8-rust-1.99.0-nightly')
error: aborting due to 1 previous error; 2 warnings emittedcodegen: handle OperandValue::Uninit in codegen_return_terminator Fixes rust-lang#159815. rust-lang#157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
codegen: handle OperandValue::Uninit in codegen_return_terminator Fixes #159815. #157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
codegen: handle OperandValue::Uninit in codegen_return_terminator Fixes #159815. #157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
codegen: handle OperandValue::Uninit in codegen_return_terminator Fixes #159815. #157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
codegen: handle OperandValue::Uninit in codegen_return_terminator Fixes rust-lang#159815. rust-lang#157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
codegen: handle OperandValue::Uninit in codegen_return_terminator Fixes #159815. #157797 added `OperandValue::Uninit` to skip stores for entirely-uninit constants, but missed the `PassMode::Direct | PassMode::Pair` branch of `codegen_return_terminator`, which called `immediate_or_packed_pair` unconditionally. A function directly returning an all-uninit value (e.g. `MaybeUninit::uninit()`) would hit that branch and ICE in `OperandRef::immediate`, as reported against tokio and reduced by tmiasko.
View all comments
MIR GVN (since #147827) propagates MaybeUninit::uninit() as
const <uninit>in aggregate constructions. Without this fix, codegen would emit a memcpy from an[N x i8] undefglobal for each such field, which LLVM materializes as zero-initialization.This mirrors the existing
all_bytes_uninitskip already present forRvalue::Use(added in #147827) into theRvalue::Aggregatefield loop.Fixes: #157743