perf(codegen): Restore noundef On PassMode::Cast Args In Rust ABI#152864
Conversation
|
|
| I'll take a look at parts of this but I can't do the full review. |
This comment has been minimized.
This comment has been minimized.
| @rustbot label +A-LLVM +A-codegen +A-ABI +C-optimization +T-compiler |
| @bjorn3 Ready for review. Thanks for the advice! |
| @bors try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…=<try> perf(codegen): Restore `noundef` On `PassMode::Cast` Args In Rust ABI
This comment has been minimized.
This comment has been minimized.
| Finished benchmarking commit (336510d): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 2.0%, secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.6%, secondary -2.2%)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: 480.206s -> 479.468s (-0.15%) |
`adjust_for_rust_abi` was casting small aggregates to an integer register without propagating `noundef`, causing a performance regression (rust-lang#123183) — LLVM could no longer assume the bits were fully defined. Add `layout_is_noundef` (conservative) + `fields_are_noundef` helper, then use `cast_to_with_attrs` to forward `NoUndef` when proven: Scalar → `!is_uninit_valid()` ScalarPair → both scalars valid + `s1.size + s2.size == layout.size` (size equality rejects layouts with inter-scalar padding) Array → recurse into element; empty arrays unconditionally noundef Arbitrary → `Variants::Single` required; walk fields in offset order — any gap, non-noundef field, or trailing pad returns false Union / Primitive / Simd → false (conservative) Bless `pass-indirectly-attr.stderr` and `debuginfo-dse.rs` for the new attribute on Cast args. Add `tests/codegen-llvm/abi-noundef-cast.rs` covering positive Cast cases (arrays, plain structs, single-variant enum) and negative Cast cases (MaybeUninit, multi-variant enum, field/pair gap, trailing padding). Fixes rust-lang#123183. Co-authored-by: Ralf Jung <post@ralfj.de>
d104c32 to 97bd985 Compare
|
| @rustbot ready |
| @bors r+ |
This comment has been minimized.
This comment has been minimized.
…=RalfJung perf(codegen): Restore `noundef` On `PassMode::Cast` Args In Rust ABI ### Summary: #### Problem: Small aggregate arguments passed via `PassMode::Cast` in the Rust ABI (e.g. `[u32; 2]` cast to `i64`) are missing `noundef` in the emitted LLVM IR, even when the type contains no uninit bytes: ```rust #[no_mangle] pub fn f(v: [u32; 2]) -> u32 { v[0] } ``` ```llvm ; expected: define i32 @f(i64 noundef %0) ; actual: define i32 @f(i64 %0) ← noundef missing ``` This blocks LLVM from applying optimizations that require value-defined semantics on function arguments. #### Root Cause: `adjust_for_rust_abi` calls `arg.cast_to(Reg::Integer)`, which internally creates a `CastTarget` with `ArgAttributes::new()` — always empty. Any validity attribute that was present before the cast is silently dropped. This affects all `PassMode::Cast` arguments and return values in the Rust ABI: plain arrays, newtype wrappers, and any `BackendRepr::Memory` type small enough to fit in a register. A prior attempt (#127210) used `Ty`/`repr` attributes to detect padding. #### Solution: After `adjust_for_rust_abi`, iterate all `PassMode::Cast` args and the return value. For each, call `layout_is_noundef` on the original layout; if it returns `true`, set `NoUndef` on the `CastTarget`'s `attrs`. `layout_is_noundef` uses only the computed layout — `BackendRepr`, `FieldsShape`, `Variants`, `Scalar::is_uninit_valid()` — and never touches `Ty` or repr attributes. **Anything it cannot prove returns `false`.** Covered cases: - `Scalar` / `ScalarPair` (both halves initialized, fields contiguous) - `FieldsShape::Array` (element type recursively uninit-free) - `FieldsShape::Arbitrary` with `Variants::Single` (fields cover `0..size` with no gaps, each recursively uninit-free) — handles newtype wrappers, multi-field structs, single-variant enums, `repr(transparent)`, `repr(C)` wrappers Conservatively excluded with FIXMEs: - Multi-variant enums (per-variant padding analysis needed) - Foreign-ABI casts (cast target may exceed layout size, needs a size guard) ### Changes: - `compiler/rustc_ty_utils/src/abi.rs`: add restoration loop after `adjust_for_rust_abi`; add `layout_is_noundef` and `fields_cover_layout`. - `tests/codegen-llvm/abi-noundef-cast.rs`: new FileCheck test covering arrays, newtype wrappers (`repr(Rust)`, `repr(transparent)`, `repr(C)`), multi-field structs, single-variant enums, return values, and negative cases (`MaybeUninit`, struct with trailing padding). - `tests/codegen-llvm/debuginfo-dse.rs`: update one CHECK pattern — `Aggregate_4xi8` (`struct { i8, i8, i8, i8 }`) now correctly gets `noundef`. Fixes #123183. r? @RalfJung | 💔 Test for a3f0079 failed: CI. Failed job:
|
| @bors retry |
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 8215374 (parent) -> c2c6f74 (this PR) Test differencesShow 8 test diffsStage 1
Stage 2
Additionally, 4 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 c2c6f74fd2216a63c7d180d5dda4c825d503c2fa --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 (c2c6f74): comparison URL. Overall result: ✅ improvements - no action needed@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 -3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.5%)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: 479.99s -> 479.752s (-0.05%) |
View all comments
Summary:
Problem:
Small aggregate arguments passed via
PassMode::Castin the Rust ABI (e.g.[u32; 2]cast toi64) are missingnoundefin the emitted LLVM IR, even when the type contains no uninit bytes:This blocks LLVM from applying optimizations that require value-defined semantics on function arguments.
Root Cause:
adjust_for_rust_abicallsarg.cast_to(Reg::Integer), which internally creates aCastTargetwithArgAttributes::new()— always empty. Any validity attribute that was present before the cast is silently dropped.This affects all
PassMode::Castarguments and return values in the Rust ABI: plain arrays, newtype wrappers, and anyBackendRepr::Memorytype small enough to fit in a register.A prior attempt (#127210) used
Ty/reprattributes to detect padding.Solution:
After
adjust_for_rust_abi, iterate allPassMode::Castargs and the return value. For each, calllayout_is_noundefon the original layout; if it returnstrue, setNoUndefon theCastTarget'sattrs.layout_is_noundefuses only the computed layout —BackendRepr,FieldsShape,Variants,Scalar::is_uninit_valid()— and never touchesTyor repr attributes. Anything it cannot prove returnsfalse.Covered cases:
Scalar/ScalarPair(both halves initialized, fields contiguous)FieldsShape::Array(element type recursively uninit-free)FieldsShape::ArbitrarywithVariants::Single(fields cover0..sizewith no gaps, each recursively uninit-free) — handles newtype wrappers, multi-field structs, single-variant enums,repr(transparent),repr(C)wrappersConservatively excluded with FIXMEs:
Changes:
compiler/rustc_ty_utils/src/abi.rs: add restoration loop afteradjust_for_rust_abi; addlayout_is_noundefandfields_cover_layout.tests/codegen-llvm/abi-noundef-cast.rs: new FileCheck test covering arrays, newtype wrappers (repr(Rust),repr(transparent),repr(C)), multi-field structs, single-variant enums, return values, and negative cases (MaybeUninit, struct with trailing padding).tests/codegen-llvm/debuginfo-dse.rs: update one CHECK pattern —Aggregate_4xi8(struct { i8, i8, i8, i8 }) now correctly getsnoundef.Fixes #123183.
r? @RalfJung