-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
rustbot has assigned @compiler-errors. Use |
tests/codegen/range-attribute.rs
Outdated
|
||
use std::num::NonZero; | ||
|
||
// CHECK: noundef range(i64 1, 0) i64 @nonzero_int(i64 noundef range(i64 1, 0) %x) |
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 think you can do
// CHECK: noundef range(i64 1, 0) i64 @nonzero_int(i64 noundef range(i64 1, 0) %x) | |
// CHECK-LABEL: @nonzero_int | |
// CHECK-SAME: noundef range(i64 1, 0) i64 @nonzero_int(i64 noundef range(i64 1, 0) %x) |
so you get the better section splitting from CHECK-LABEL if the line doesn't match for some reason.
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.
CHECK-SAME will start matching after the CHECK-LABEL so it is not possible to verify that the result get a range applied in that case unless it is moved to the CHECK-LABEL like
CHECK-LABEL: noundef range(i64 1, 0) i64 @nonzero_int
and then it feels like a CHECK is better
let apply_range_attr = |idx: AttributePlace, abi: rustc_target::abi::Abi| { | ||
if cx.sess().opts.optimize != config::OptLevel::No | ||
&& llvm_util::get_version() >= (19, 0, 0) | ||
{ | ||
if let abi::Abi::Scalar(scalar) = abi { | ||
// If the value is a boolean, the range is 0..2 and that ultimately | ||
// become 0..0 when the type becomes i1, which would be rejected | ||
// by the LLVM verifier. | ||
if let Int(..) = scalar.primitive() { | ||
if !scalar.is_bool() && !scalar.is_always_valid(cx) { |
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.
Don't quote me on this but I think it is okay to use let chains in the compiler to avoid all this nesting
let apply_range_attr = |idx: AttributePlace, abi: rustc_target::abi::Abi| { | |
if cx.sess().opts.optimize != config::OptLevel::No | |
&& llvm_util::get_version() >= (19, 0, 0) | |
{ | |
if let abi::Abi::Scalar(scalar) = abi { | |
// If the value is a boolean, the range is 0..2 and that ultimately | |
// become 0..0 when the type becomes i1, which would be rejected | |
// by the LLVM verifier. | |
if let Int(..) = scalar.primitive() { | |
if !scalar.is_bool() && !scalar.is_always_valid(cx) { | |
let apply_range_attr = |idx: AttributePlace, abi: rustc_target::abi::Abi| { | |
if cx.sess().opts.optimize != config::OptLevel::No | |
&& llvm_util::get_version() >= (19, 0, 0) | |
&& let abi::Abi::Scalar(scalar) = abi | |
// If the value is a boolean, the range is 0..2 and that ultimately | |
// become 0..0 when the type becomes i1, which would be rejected | |
// by the LLVM verifier. | |
&& matches!(scalar.primitive(), Int(..)) | |
&& !scalar.is_bool() && !scalar.is_always_valid(cx) { |
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.
Yes please
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.
One day that will stable :(
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.
nice did not think it was possible to do that.
Marking as blocked on #127513, which hopefully won't be too far away. Also, thanks for pushing this forward on both the Rust and LLVM side. The results will be awesome :) |
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.
It's great to see this!
Ideally you'll be able to revert the assumes added in #103584, now that we can represent this range info properly. (in another PR, perhaps)
// Checks that range metadata gets emitted on functions result and arguments | ||
// with scalar value. |
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.
Some more tests that would be nice:
- slice length (I don't think that will get range attributes yet, but when it does, it'll resolve Add unreachable hints for length getters? #106737. Having a test is still useful whether it gets the attribute or not, just to show the current state.)
- discriminant of an enum with a payload (one that gets scalarpair layout) e.g.
enum Foo { A(u64), B(u64), C(u64) }
.
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.
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 assume
s was perf-horrible.
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.
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.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization. r? `@nikic`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b8ab1d7): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 3.4%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 755.634s -> 754.893s (-0.10%) |
5ff174c
to
009b4e2
Compare
This is not blocked anymore now after the rebase and the perf was ok. |
@@ -20,8 +20,6 @@ pub fn u32_index(c: u32) -> [bool; 22] { | |||
// CHECK-LABEL: @char_as_u32_index | |||
#[no_mangle] | |||
pub fn char_as_u32_index(c: char) -> [bool; 22] { | |||
// CHECK: %[[B:.+]] = icmp ult i32 %c, 1114112 |
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.
@rustbot ready |
tests/codegen/range-attribute.rs
Outdated
|
||
// CHECK: noundef range(i8 0, 4) i8 @enum1_value(i8 noundef range(i8 0, 4) %x) | ||
#[no_mangle] | ||
pub fn enum1_value(x: Enum0) -> Enum0 { |
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.
pub fn enum1_value(x: Enum0) -> Enum0 { | |
pub fn enum1_value(x: Enum1) -> Enum1 { |
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.
fixed
009b4e2
to
12f8b2d
Compare
tests/codegen/range-attribute.rs
Outdated
// CHECK: define { i64, i64 } @enum1_value(i64 noundef %x.0, i64 noundef %x.1) | ||
#[no_mangle] | ||
pub fn enum1_value(x: Enum1) -> Enum1 { | ||
x | ||
} |
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.
Hmm, this should be getting a range attribute, we definitely set the valid range for discriminants. I suspect...
let apply_range_attr = |idx: AttributePlace, abi: rustc_target::abi::Abi| { | ||
if cx.sess().opts.optimize != config::OptLevel::No | ||
&& llvm_util::get_version() >= (19, 0, 0) | ||
&& let abi::Abi::Scalar(scalar) = abi | ||
&& matches!(scalar.primitive(), Int(..)) | ||
// If the value is a boolean, the range is 0..2 and that ultimately | ||
// become 0..0 when the type becomes i1, which would be rejected | ||
// by the LLVM verifier. | ||
&& !scalar.is_bool() | ||
// LLVM also rejects full range. | ||
&& !scalar.is_always_valid(cx) | ||
{ | ||
attributes::apply_to_llfn( | ||
llfn, | ||
idx, | ||
&[llvm::CreateRangeAttr(cx.llcx, scalar.size(cx), scalar.valid_range(cx))], | ||
); | ||
} | ||
}; |
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.
...that this isn't called in enough places. Instead of building the attribute here, I would suggest:
- add a
valid_range
field tostruct ArgAttributes
- set
valid_range
inadjust_for_rust_scalar
when applicable - call
llvm::CreateRangeAttr
inrustc_codegen_llvm::abi::get_attrs
, based on the value ofvalid_range
Then the existing code should handle the rest.
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 solution seems to use a lot more memory if I add a valid_range_and_num_bits: Option<(WrappingRange, u32)> to ArgAttributes the ArgAbi size goes from 56 to 192 and FnAbi from 80 to 224 and the WrappingRange will not be interned but the layout is so even the same types will result in same ranges manytimes.
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.
updated scalarPair also to avoid undoing the work from #100999 but there maybe is some good reason to do like you suggested like other backends want to use the data also.
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.
Is the size of ArgAttributes / FnAbi sensitive? I'd have hoped they only exist temporarily while lowering a call/declaration, but maybe that's not the case.
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.
ended up at this assert and did not know of a good reason to increase it.
https://github.com/andjo403/rust/blob/3cbfa8212a48f8eecce9c5da0615da6c4c883ce9/compiler/rustc_target/src/abi/call/mod.rs#L970-L980
12f8b2d
to
5cc0300
Compare
This comment has been minimized.
This comment has been minimized.
5cc0300
to
3cbfa82
Compare
@bors r+ |
01a3604
to
cfadfab
Compare
managed to run the |
@bors r+ |
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e08b80c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -2.5%, secondary 2.1%)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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (secondary 0.0%)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.
Bootstrap: 753.683s -> 753.452s (-0.03%) |
Stop generating assumes for validity ranges After rust-lang#128371, we represent validity ranges as parameter / return value attributes, so we no longer need to use assumes. r? `@ghost`
Pinging back #127883. |
as LLVM 19 adds the range attribute this starts to use it for better optimization.
hade been interesting to see a perf run with the #127513
closes #50156
cc #49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.