Use nuw when calculating slice lengths from Ranges
#108763
Conversation
|
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment was marked as resolved.
This comment was marked as resolved.
|
To double-check that the flag really is cheap to add, |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit be2d01aaf9721c0bc7fc0c8cbc622bcc3e98f98d with merge 430c05dbb9a23df78dfb78aedbdedfd47710f194... |
|
Conveniently this also lets us catch some const A: [(); 5] = [(), (), (), (), ()];
const B: &[()] = unsafe { A.get_unchecked(3..1) };
dbg!(B.len()); // 18446744073709551614 today, uhohI'll add a UI test for that once perf is done. |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (430c05dbb9a23df78dfb78aedbdedfd47710f194): 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. CyclesResultsThis 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.
|
rustbot
removed
the
S-waiting-on-perf
scottmcm
force-pushed
the
indexing-nuw-lengths
branch
from
be2d01a
to
5417e9d
Compare
|
I was just hoping for neutral, so getting a bunch of green is a pleasant surprise. Awesome that this apparently helps something in the compiler. I've also pushed the UI test to confirm that CTFE now catches more UB (though it's only reachable in |
This comment was marked as resolved.
This comment was marked as resolved.
An `assume` would definitely not be worth it, but since the flag is almost free we might as well tell LLVM this, especially on `_unchecked` calls where there's no obvious way for it to deduce it. (Today neither safe nor unsafe indexing gets it: <https://rust.godbolt.org/z/G1jYT548s>)
scottmcm
force-pushed
the
indexing-nuw-lengths
branch
from
5417e9d
to
3554036
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (160c2eb): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
…scottmcm Use unchecked_sub in str indexing rust-lang#108763 applied this logic to indexing for slices, but of course `str` has its own separate impl. Found this by skimming over the codegen for https://github.com/oxidecomputer/hubris/; their dist builds enable overflow checks so the lack of `unchecked_sub` was producing an impossible-to-hit overflow check and also inhibiting some inlining. r? scottmcm
Use unchecked_sub in str indexing rust-lang/rust#108763 applied this logic to indexing for slices, but of course `str` has its own separate impl. Found this by skimming over the codegen for https://github.com/oxidecomputer/hubris/; their dist builds enable overflow checks so the lack of `unchecked_sub` was producing an impossible-to-hit overflow check and also inhibiting some inlining. r? scottmcm
…_use_unchecked, r=Nilstrieb Use `unchecked_sub` in `split_at` LLVM currently isn't figuring it out on its own, even in the checked version where it hypothetically could. Before: <https://rust.godbolt.org/z/PEY38YrKs> ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub i64 %x.1, %n ``` After: ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub nuw i64 %x.1, %n ``` This is not using the wrapper because there's already a ubcheck covering it, so I don't want this to get a second one once rust-lang#121571 lands. --- This is basically the same as rust-lang#108763, since `split_at` is essentially doing two `get_unchecked`s.
…_use_unchecked, r=Nilstrieb Use `unchecked_sub` in `split_at` LLVM currently isn't figuring it out on its own, even in the checked version where it hypothetically could. Before: <https://rust.godbolt.org/z/PEY38YrKs> ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub i64 %x.1, %n ``` After: ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub nuw i64 %x.1, %n ``` This is not using the wrapper because there's already a ubcheck covering it, so I don't want this to get a second one once rust-lang#121571 lands. --- This is basically the same as rust-lang#108763, since `split_at` is essentially doing two `get_unchecked`s.
Rollup merge of rust-lang#124699 - scottmcm:split_at_unchecked_should_use_unchecked, r=Nilstrieb Use `unchecked_sub` in `split_at` LLVM currently isn't figuring it out on its own, even in the checked version where it hypothetically could. Before: <https://rust.godbolt.org/z/PEY38YrKs> ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub i64 %x.1, %n ``` After: ```llvm bb1: ; preds = %start %4 = getelementptr inbounds float, ptr %x.0, i64 %n %5 = sub nuw i64 %x.1, %n ``` This is not using the wrapper because there's already a ubcheck covering it, so I don't want this to get a second one once rust-lang#121571 lands. --- This is basically the same as rust-lang#108763, since `split_at` is essentially doing two `get_unchecked`s.
An
assumewould definitely not be worth it, but since the flag is almost free we might as well tell LLVM this, especially on_uncheckedcalls where there's no obvious way for it to deduce it.(Today neither safe nor unsafe indexing gets it: https://rust.godbolt.org/z/G1jYT548s)