Annotate eligible small immediate arguments with `noundef` #127210

jieyouxu · 2024-07-01T15:39:07Z

Retake of #123425.

We try to annotate small (fits within target pointer width) aggregate arguments passed as immediates
(specifically casted as "appropriately sized integer type") with noundef.

Example:

#[no_mangle]
pub fn short_array_u64x1(v: [u64; 1]) -> [u64; 1] { v }

currently produces

define i64 @short_array_u64x1(i64 %0) ...

This PR changes that to

define noundef i64 @short_array_u64x1(i64 noundef %0) ...

The noundef attribute is added only if the immediate value has no padding. Specifically,
the conservative heuristic we use is to:

Peel away layers of #[repr(Rust)] or #[repr(transparent)] wrappers if present
Check for innermost simple arrays (whose element type is a primitive type) that can fit within
target pointer width

Union immediates or otherwise anything that contains unions will not have noundef attribute
applied.

Closes #123183.

cc @RalfJung who pointed out various problems with the previous take, hopefully I addressed most of
them in this take.

r? @ghost (perf)

Add both positive and negative test cases for adding `noundef` to immediate arguments that fit within target pointer width.

jieyouxu · 2024-07-01T15:39:24Z

@rustbot author

jieyouxu · 2024-07-01T15:39:44Z

@bors try @rust-timer queue

bors · 2024-07-01T15:40:54Z

⌛ Trying commit 80171db with merge caa50bd...

…<try> Annotate eligible small immediate arguments with `noundef` Retake of rust-lang#123425. We try to annotate small (fits within target pointer width) aggregate arguments passed as immediates (specifically casted as "appropriately sized integer type") with `noundef`. Example: ```rs #[no_mangle] pub fn short_array_u64x1(v: [u64; 1]) -> [u64; 1] { v } ``` currently produces ```llvm define i64 `@short_array_u64x1(i64` %0) ... ``` This PR changes that to ```llvm define noundef i64 `@short_array_u64x1(i64` noundef %0) ... ``` The `noundef` attribute is added only if the immediate value has no padding. Specifically, the conservative heuristic we use is to: - Peel away layers of `#[repr(Rust)]` or `#[repr(transparent)]` wrappers if present - Check for innermost simple arrays (whose element type is a primitive type) that can fit within target pointer width Union immediates or otherwise anything that contains unions will not have `noundef` attribute applied. Closes rust-lang#123183. cc `@/RalfJung` who pointed out various problems with the previous take, hopefully I addressed most of them in this take. r? `@ghost` (perf)

jieyouxu · 2024-07-01T15:47:20Z

compiler/rustc_ty_utils/src/abi.rs

@@ -822,6 +835,44 @@ fn fn_abi_adjust_for_abi<'tcx>(
    Ok(())
 }

+fn can_annotate_small_immediate_argument_with_noundef<'tcx>(


I'm not super confident in the correctness of this eligibility check here. I've added positive and negative test cases for edge cases like unions or array of unions, but I still feel like I'm missing cases. In any case, this is a conservative heuristic for checking "does this immediate have padding".

bors · 2024-07-01T17:20:27Z

☀️ Try build successful - checks-actions
Build commit: caa50bd (caa50bd18970388efed30665a39a4b12f11b0ae7)

rust-timer · 2024-07-01T18:51:09Z

Finished benchmarking commit (caa50bd): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.9%)

This 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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 697.337s -> 698.097s (0.11%)
Artifact size: 326.67 MiB -> 326.70 MiB (0.01%)

jieyouxu · 2024-07-01T22:47:12Z

r? @scottmcm or compiler

jieyouxu · 2024-07-01T22:54:08Z

This should now be ready for review. @rustbot ready

compiler/rustc_abi/src/lib.rs

Add `noundef` metadata for arguments that: 1. are passed as immediates 2. fit in target pointer width 3. has no padding

RalfJung · 2024-07-02T13:10:28Z

Peel away layers of #[repr(Rust)] or #[repr(transparent)] wrappers if present

What's a repr(Rust) wrapper? The property you are after is "is there any padding or a union inside of this". repr(Rust) types can have padding.

Also since this is not about stable layout guarantees, I wouldn't bother with the repr. Instead, I'd suggest to call non_1zst_field which will return the unique non-1-ZST field in a type. If that function returns Some on a struct, you are guaranteed that there is no padding around that field, so you can continue the recursive check.

RalfJung · 2024-07-02T13:12:48Z

compiler/rustc_ty_utils/src/abi.rs

+        if layout.ty.is_union() {
+            return false;
+        }


You can avoid having to duplicate this check if you put it before the non_1zst_field call.

Also, what about enums?

RalfJung · 2024-07-02T13:13:55Z

tests/codegen/noundef-small-immediate-arguments.rs

+// NoOpt: define i32 @repr_c_immediate(i32 %0)
+// Opt: define i32 @repr_c_immediate(i32 %0)
+#[no_mangle]
+pub fn repr_c_immediate(v: ReprCWrapper) -> ReprCWrapper {


I don't see any reason why this could not be noundef

scottmcm · 2024-07-02T15:53:31Z

Also since this is not about stable layout guarantees, I wouldn't bother with the repr.

I agree with Ralf on this. I think you probably want to look just at the actual computed layout, completely ignoring the original rust code that resulted in it. (That way it's immune to any changes to repr(Rust), too.)

Also, since this is basically just checking the same thing as https://docs.rs/bytemuck/latest/bytemuck/trait.NoUninit.html and https://docs.rs/zerocopy/latest/zerocopy/trait.AsBytes.html, it might be worth seeing if any of the safe transmute work already has code that you can just steal to do this.

@rustbot author

jieyouxu · 2024-07-03T11:59:23Z

I agree with Ralf on this. I think you probably want to look just at the actual computed layout, completely ignoring the original rust code that resulted in it. (That way it's immune to any changes to repr(Rust), too.)

That seems more correct to me than the current approach yes.

Also, since this is basically just checking the same thing as docs.rs/bytemuck/latest/bytemuck/trait.NoUninit.html and docs.rs/zerocopy/latest/zerocopy/trait.AsBytes.html, it might be worth seeing if any of the safe transmute work already has code that you can just steal to do this.

Thanks for the pointers (heh). NoUninit's analysis looks like something I want to adapt from, possibly more conservative.

tests: add test cases for adding noundef to small immediate arguments

d0438b4

Add both positive and negative test cases for adding `noundef` to immediate arguments that fit within target pointer width.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 1, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 1, 2024

jieyouxu commented Jul 1, 2024

View reviewed changes

jieyouxu added the A-codegen Area: Code generation label Jul 1, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 1, 2024

rustbot assigned scottmcm Jul 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2024

the8472 reviewed Jul 1, 2024

View reviewed changes

compiler/rustc_abi/src/lib.rs Outdated Show resolved Hide resolved

codegen: add noundef for small immediate arguments with no padding

3704226

Add `noundef` metadata for arguments that: 1. are passed as immediates 2. fit in target pointer width 3. has no padding

rust-cloud-vms bot force-pushed the array-imm-noundef-param branch from 80171db to 3704226 Compare July 2, 2024 00:52

RalfJung reviewed Jul 2, 2024

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2024

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate eligible small immediate arguments with `noundef` #127210

Annotate eligible small immediate arguments with `noundef` #127210

jieyouxu commented Jul 1, 2024 •

edited

Loading

jieyouxu commented Jul 1, 2024

jieyouxu commented Jul 1, 2024

This comment has been minimized.

bors commented Jul 1, 2024

jieyouxu Jul 1, 2024 •

edited

Loading

bors commented Jul 1, 2024

This comment has been minimized.

rust-timer commented Jul 1, 2024

jieyouxu commented Jul 1, 2024

jieyouxu commented Jul 1, 2024

RalfJung commented Jul 2, 2024

RalfJung Jul 2, 2024

RalfJung Jul 2, 2024

scottmcm commented Jul 2, 2024

jieyouxu commented Jul 3, 2024

Annotate eligible small immediate arguments with noundef #127210

Are you sure you want to change the base?

Annotate eligible small immediate arguments with noundef #127210

Conversation

jieyouxu commented Jul 1, 2024 • edited Loading

jieyouxu commented Jul 1, 2024

jieyouxu commented Jul 1, 2024

This comment has been minimized.

bors commented Jul 1, 2024

jieyouxu Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

bors commented Jul 1, 2024

This comment has been minimized.

rust-timer commented Jul 1, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

jieyouxu commented Jul 1, 2024

jieyouxu commented Jul 1, 2024

RalfJung commented Jul 2, 2024

RalfJung Jul 2, 2024

Choose a reason for hiding this comment

RalfJung Jul 2, 2024

Choose a reason for hiding this comment

scottmcm commented Jul 2, 2024

jieyouxu commented Jul 3, 2024

Annotate eligible small immediate arguments with `noundef` #127210

Annotate eligible small immediate arguments with `noundef` #127210

jieyouxu commented Jul 1, 2024 •

edited

Loading

jieyouxu Jul 1, 2024 •

edited

Loading