JIT: Opportunistically lower value & ((1 << k) - 1) to BMI2 bzhi - #131693
JIT: Opportunistically lower value & ((1 << k) - 1) to BMI2 bzhi#131693tcortega wants to merge 3 commits into
value & ((1 << k) - 1) to BMI2 bzhi#131693Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adds an xarch JIT lowering pattern that recognizes value & ((1 << k) - 1)-style masks and opportunistically replaces the AND + (shift/dec) mask construction with HWIntrinsic::ZeroHighBits (emitting bzhi when supported), plus a regression test covering boundary shift-count semantics.
Changes:
- Add
Lowering::TryLowerAndOpToZeroHighBitsand invoke it fromLowerBinaryArithmeticforGT_AND. - Add a new JitBlue regression test
Runtime_129368and include it in the regression csproj. - Declare the new lowering helper in
lower.h.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/lowerxarch.cpp | Adds the new AND( value, (1<<k)-1 ) → ZeroHighBits lowering and wires it into lowering. |
| src/coreclr/jit/lower.h | Declares TryLowerAndOpToZeroHighBits. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new regression test source file. |
| src/tests/JIT/Regression/JitBlue/Runtime_129368/Runtime_129368.cs | Adds coverage for shift-count edge cases and various shape variants. |
79c245c to
ecef081
Compare
|
Good catch - this was real. Turns out the existing blsr/blsmsk lowers have the same hole ( |
ecef081 to
5582b4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/coreclr/jit/lowerxarch.cpp:6699
- This JITDUMP message refers only to the SUB form, but the transform also matches the ADD(..., -1) canonicalization. Consider wording it in terms of the source-level mask to keep diagnostics accurate.
JITDUMP("Lower: optimize AND(X, SUB(LSH(1, CNT), 1))\n");
src/coreclr/jit/lowerxarch.cpp:6586
- The header comment describes only the SUB form, but the implementation also accepts the morph-canonicalized ADD(..., -1) form. Updating the comment avoids misleading future readers/debugging.
This issue also appears on line 6699 of the same file.
// Lowering::TryLowerAndOpToZeroHighBits: Lowers a tree AND(X, SUB(LSH(1, CNT), 1)) to
// HWIntrinsic::ZeroHighBits
|
I'm not sure it's worth the extra complexity in JIT for very small diffs. Is it used in real-world or is there any benchmark? |
|
@EgorBo Fair question. The diffs undercount it a bit - constant On the TP diff: it only shows up on the MSVC-built jit (clang linux-x64 is clean), and MinOpts regresses too even though this sits behind No benchmark yet, happy to add one if that helps. Design is from #129368 so I'll defer to @AndyAyersMS on whether it clears the bar. |
|
This is already covered by #130481 Which is not only bzhi, but also bts, btr, btc, bt, and ensuring logic sharing with LA64 and other architectures where relevant. That's just pending sign-off (@EgorBo) and is otherwise fine to be merged.
Same general response as I gave on the other PR, which is that these are standard bit manipulation intrinsics supported by all 3 modern compilers when targeting hardware with such support; they are exposed because they are known to be faster/better, shortening dependency chains and reducing complexity. Its also a bit of a chicken and egg problem; in many cases alternative patterns get used because we aren't accelerating these standard defaults. If we do accelerate them, its more likely people will use them and less likely they have to code complex or risky custom logic to accelerate to account for our lack of handling it. |
Fixes #129368
Lowers
value & ((1 << k) - 1)(and theADD(..., -1)form) to BMI2bzhiwhen available.BZHI leaves the source alone when
k >= width, IL shifts mask the count. If we can't provekis in range, insertand k, width-1so the behavior stays the same.Based on @AndyAyersMS's prototype. Left out the
IntegralRangefallback - it almost never fires in lowering, and the issue already calls that out as follow-up.SPMI windows-x64 Checked: 45 diffs, -315 bytes, 0 size regressions.
Test:
Runtime_129368sweeps edgekvalues againstNoOptimizationoracles.Update: overflow-checked masks (
checked((1 << k) - 1)) now bail instead of dropping the throw, and the existing blsr/blsmsk lowers turned out to have the same hole - guarded those too.