-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add range attribute to scalar function results and arguments #128371
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
// Checks that range metadata gets emitted on functions result and arguments | ||
// with scalar value. | ||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some more tests that would be nice:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks to rust-lang/reference#1499 we're allowed to do this for slices, and per #122926 (comment) it has a decent chance to save a couple megs off the compiler size. So excited to get to do it with range attributes, since doing it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. think that I have added the test that you wanted. I'm letting someone more familiar create a PR for adding the range on slices. |
||
|
||
//@ compile-flags: -O -C no-prepopulate-passes | ||
//@ min-llvm-version: 19 | ||
|
||
#![crate_type = "lib"] | ||
|
||
use std::num::NonZero; | ||
|
||
// Hack to get the correct size for usize | ||
// CHECK: @helper([[USIZE:i[0-9]+]] noundef %_1) | ||
#[no_mangle] | ||
pub fn helper(_: usize) {} | ||
|
||
// CHECK: noundef range(i128 1, 0) i128 @nonzero_int(i128 noundef range(i128 1, 0) %x) | ||
#[no_mangle] | ||
pub fn nonzero_int(x: NonZero<u128>) -> NonZero<u128> { | ||
x | ||
} | ||
|
||
// CHECK: noundef range(i8 0, 3) i8 @optional_bool(i8 noundef range(i8 0, 3) %x) | ||
#[no_mangle] | ||
pub fn optional_bool(x: Option<bool>) -> Option<bool> { | ||
x | ||
} | ||
|
||
pub enum Enum0 { | ||
A(bool), | ||
B, | ||
C, | ||
} | ||
|
||
// CHECK: noundef range(i8 0, 4) i8 @enum0_value(i8 noundef range(i8 0, 4) %x) | ||
#[no_mangle] | ||
pub fn enum0_value(x: Enum0) -> Enum0 { | ||
x | ||
} | ||
|
||
pub enum Enum1 { | ||
A(u64), | ||
B(u64), | ||
C(u64), | ||
} | ||
|
||
// CHECK: { [[ENUM1_TYP:i[0-9]+]], i64 } @enum1_value([[ENUM1_TYP]] noundef range([[ENUM1_TYP]] 0, 3) %x.0, i64 noundef %x.1) | ||
#[no_mangle] | ||
pub fn enum1_value(x: Enum1) -> Enum1 { | ||
x | ||
} | ||
|
||
pub enum Enum2 { | ||
A(Enum0), | ||
B(Enum0), | ||
C(Enum0), | ||
} | ||
|
||
// CHECK: { i8, i8 } @enum2_value(i8 noundef range(i8 0, 3) %x.0, i8 noundef %x.1) | ||
#[no_mangle] | ||
pub fn enum2_value(x: Enum2) -> Enum2 { | ||
x | ||
} | ||
|
||
// CHECK: noundef [[USIZE]] @takes_slice(ptr noalias noundef nonnull readonly align 4 %x.0, [[USIZE]] noundef %x.1) | ||
#[no_mangle] | ||
pub fn takes_slice(x: &[i32]) -> usize { | ||
x.len() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is optimized away when the range is added to the input. I only removed it as it do not seem important for the test and more a nice to have check. otherwise I do not know how to update this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the
CHECK-NOT: call core::panicking::panic
below, ensuring that the bounds check gets optimized out, is the important part of this test. Since that still works, I think the spirit of this test is upheld.You can/should remove the assume (added in https://github.com/rust-lang/rust/pull/124905/files#diff-db08b7c8b00530c7183cf2e42f25dc93b02da93fb40edadbd009eee6ad60e3a1R298-R302) now that it gets optimized out anyways. (Probably need to add
min-llvm-version
to the test in that case, which is fine.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do that assume not add value if the cast is not on an input/result of a function call ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave it there for now, I'll take a look at it in a separate PR.
Technically yes, it could be beneficial. Although, after this PR, we'll have range metadata on loads, arguments, and returns, which covers the vast majority of cases where new values are "introduced" into a function. I think it's unlikely that handing the remaining rare cases would be worthwhile, given that adding assumes costs a bit of compile time and can block other optimizations.