Rework next_power_of_two to always be 1 << … - #161209
Conversation
| // Slices (of non-ZSTs) are short enough that the power-of-two always fits | ||
| #[unsafe(no_mangle)] | ||
| pub fn slice_length_npot(slice: &[u8]) -> usize { | ||
| // CHECK-LABEL: @slice_length_npot( | ||
| // CHECK: [[POT:%.+]] = shl nuw i64 1, | ||
| // CHECK: ret i64 [[POT]] | ||
| slice.len().next_power_of_two() | ||
| } |
There was a problem hiding this comment.
Previously on x86 this had a branch for len < 2: https://rust.godbolt.org/z/TdE5GjvKa
In the new one it has no branches (just a cmov for the saturating_sub).
This comment has been minimized.
This comment has been minimized.
| #[unsafe(no_mangle)] | ||
| pub unsafe fn restricted_npot(x: u16) -> u16 { | ||
| std::hint::assert_unchecked(1 <= x); | ||
| std::hint::assert_unchecked(x <= 11111); |
There was a problem hiding this comment.
This 11111 makes me a bit upset because at first I think it's binary, then I remember it's decimal. I guess this is so you're less than 2^15?
There was a problem hiding this comment.
Yeah, that was too arbitrary. Updated to the actual threshold value for the CHECK.
|
Worth adding the test for |
|
@rustbot author |
|
@SomeFlyingThing Can you say what you were doing that didn't optimize as expected that had you wanting the |
i didnt have a specific reason i was just looking to make know invariants more explicit to the optimizer so maybe downstream optimizations could make use of them |
It doesn't. Thoughts on whether we want to |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Maybe npot>=self might help some bounds checks somewhere but idk, seems unlikely. I'm fine calling this good enough. |
|
To me it's the "unsigned smaller" cases that are most useful for bounds checks so that someone calculating something off a length (or range of in-bound indexes) is also known to be in-bounds. If this was previous power of two then not having a bounds-check in |
|
@bors r+ rollup Thank you! |
|
Actually: @bors r- @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
This pull request was unapproved. |
This comment has been minimized.
This comment has been minimized.
Rework `next_power_of_two` to always be `1 << …`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0a90f9f): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -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)This perf run didn't have relevant results for this metric. CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.539s -> 474.052s (-0.31%) |
|
@bors r+ Perf nothing-burger. |
Rework `next_power_of_two` to always be `1 << …` r? @clarfonthey Who accidentally nerd-sniped me by mentioning rust-lang#161069 This obviates that PR by reworking the `(checked_)next_power_of_two` logic to calculate the necessary exponent e, then return 2ᵉ, so it's structurally obvious from the IR -- the `shl nuw 1, …` instruction -- that it's always a power of two. As always, the reason this is tricky is because shifts don't work for `<< Self::BITS`. The previous code handled that by checking `self <= 1` first, and thus doing a `-1 >> n` where `n < BITS`. This code instead flips the check: it looks up-front for a value that will wrap (an input above `1 << (BITS - 1)`) and thus by excluding those cases the calculated `1 << n` always has `n < BITS`. And the codegen tests show that, without needing any `llvm.assume`s, LLVM can take advantage of it to do things like rewriting `%` to masking. Plus the overflow check optimizes out for constrained inputs like slice lengths.
…uwer Rollup of 11 pull requests Successful merges: - #151618 (rustdoc: add `--print` option) - #161287 (Update `icu_list` dependency to 2.3) - #161767 (change DEFAULT_STACK_SIZE to be 32MB on s390x) - #161968 (Diverse offload fixes) - #161971 (Remove -Zsaturating-float-casts flag) - #162071 (Fix ICE of getting item name from RPITIT) - #161209 (Rework `next_power_of_two` to always be `1 << …`) - #162073 (Change some `Infallible` to `!` in std) - #162086 (Remove `gate_check` from `AttributeStability::Unstable`) - #162102 (`alloc` crate: shrink undocumented `unsafe` blocks) - #162110 (make it clear that Range cannot represent arbitrary ranges)
Rollup merge of #161209 - scottmcm:redo-npot, r=clarfonthey Rework `next_power_of_two` to always be `1 << …` r? @clarfonthey Who accidentally nerd-sniped me by mentioning #161069 This obviates that PR by reworking the `(checked_)next_power_of_two` logic to calculate the necessary exponent e, then return 2ᵉ, so it's structurally obvious from the IR -- the `shl nuw 1, …` instruction -- that it's always a power of two. As always, the reason this is tricky is because shifts don't work for `<< Self::BITS`. The previous code handled that by checking `self <= 1` first, and thus doing a `-1 >> n` where `n < BITS`. This code instead flips the check: it looks up-front for a value that will wrap (an input above `1 << (BITS - 1)`) and thus by excluding those cases the calculated `1 << n` always has `n < BITS`. And the codegen tests show that, without needing any `llvm.assume`s, LLVM can take advantage of it to do things like rewriting `%` to masking. Plus the overflow check optimizes out for constrained inputs like slice lengths.
View all comments
r? @clarfonthey
Who accidentally nerd-sniped me by mentioning #161069
This obviates that PR by reworking the
(checked_)next_power_of_twologic to calculate the necessary exponent e, then return 2ᵉ, so it's structurally obvious from the IR -- theshl nuw 1, …instruction -- that it's always a power of two.As always, the reason this is tricky is because shifts don't work for
<< Self::BITS. The previous code handled that by checkingself <= 1first, and thus doing a-1 >> nwheren < BITS. This code instead flips the check: it looks up-front for a value that will wrap (an input above1 << (BITS - 1)) and thus by excluding those cases the calculated1 << nalways hasn < BITS. And the codegen tests show that, without needing anyllvm.assumes, LLVM can take advantage of it to do things like rewriting%to masking. Plus the overflow check optimizes out for constrained inputs like slice lengths.No LLM used.