Recognize bit-manipulation idioms and emit bzhi/bts/btc/btr/bt on xarch#130481
Open
tannergooding wants to merge 8 commits into
Open
Recognize bit-manipulation idioms and emit bzhi/bts/btc/btr/bt on xarch#130481tannergooding wants to merge 8 commits into
tannergooding wants to merge 8 commits into
Conversation
Lowers several single-bit and bit-field idioms to dedicated x86 instructions: x & ((1 << y) - 1) -> bzhi (reuses NI_AVX2_ZeroHighBits) x | (1 << y) -> bts (GT_BIT_SET) x & ~(1 << y) -> btr (GT_BIT_CLEAR) x ^ (1 << y) -> btc (GT_BIT_INVERT) bzhi is recognized in LowerBinaryArithmetic and folded into the existing AVX2 scalar intrinsic, so containment and memory operands work for free. It matches both the ADD(-1) and SUB(1) mask forms and only fires for a variable shift amount, since a constant amount folds to a plain and with an immediate. The bts/btr/btc patterns reuse the existing GT_BIT_SET/GT_BIT_CLEAR/GT_BIT_INVERT opers (shared with riscv64) with xarch codegen, LSRA, and emitter support. Only the reg,reg form is used: the value operand is kept in a register so the implicit masking of the bit index (mod the operand width) matches the shl semantics, and because the memory-destination forms are slower. They likewise only fire for a variable bit index, since a constant index folds to a plain or/and/xor with an immediate. bextr (constant-control field extract) is intentionally left out: it must materialize the control word and is multi-uop, so shr/sar + and in place is smaller and faster; the fully-variable (x >> y) & ((1 << z) - 1) case already lowers optimally to shrx + bzhi. This can be revisited with a cost/liveness-aware heuristic in a follow-up. Contributes to dotnet#81514 Fixes dotnet#81512 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extend tryReduceSingleBitTestOps to also match AND(RSH|RSZ(x, y), 1) so that a single-bit test written as ((x >> y) & 1) feeding a branch lowers to bt, the same as the already-handled x & (1 << y) shape. Only bit 0 of the shifted value is kept so the shift kind is irrelevant, and bt masks the bit index modulo the operand size, matching the C# masked-shift semantics even for an out-of-range y. Restricted to a variable index since a constant index keeps the shift and bt has no immediate form here (the existing constant-mask test is already optimal). The value operand's containment is cleared because ContainCheckCompare is skipped on success and the reg,reg bt form requires it in a register. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends xarch JIT lowering/codegen to recognize common single-bit and bit-field idioms and emit dedicated x86 instructions (bts/btr/btc, bt, and bzhi via the existing ZeroHighBits intrinsic). It also adds/updates JIT tests to validate the new instruction selection.
Changes:
- Add lowering for single-bit set/clear/invert idioms to new
GT_BIT_SET/GT_BIT_CLEAR/GT_BIT_INVERTnodes and emitbts/btr/btcin xarch codegen. - Extend compare/test optimization to recognize the
((x >> y) & 1)shape and lower it toGT_BITTEST_EQ/NE(xarchbt). - Add/adjust instruction-combining tests, including disasm checks for the new bit ops and a new
bzhicorrectness test.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/opt/InstructionCombining/SingleBit.csproj | Enables disasm checks for SingleBit.cs. |
| src/tests/JIT/opt/InstructionCombining/SingleBit.cs | Adds disasm-validated coverage for bts/btr/btc and bt, plus 64-bit variants. |
| src/tests/JIT/opt/InstructionCombining/Bzhi.csproj | Adds a new instruction-combining test project for bzhi coverage. |
| src/tests/JIT/opt/InstructionCombining/Bzhi.cs | Adds randomized correctness checks intended to cover x & ((1 << y) - 1) lowering. |
| src/coreclr/jit/lsraxarch.cpp | Teaches LSRA to treat GT_BIT_* nodes as binary ops for use/def modeling. |
| src/coreclr/jit/lowerxarch.cpp | Adds new pattern-based lowering for bzhi and GT_BIT_* nodes; adjusts some existing bit-idiom lowering to accept SUB forms. |
| src/coreclr/jit/lower.h | Declares the new xarch lowering helpers. |
| src/coreclr/jit/lower.cpp | Extends const-compare optimization to recognize ((x >> y) & 1) for bit-test lowering. |
| src/coreclr/jit/instrsxarch.h | Adds instruction definitions for bts/btr/btc. |
| src/coreclr/jit/emitxarch.cpp | Adjusts instruction display for BT-family operand reversal for bts/btr/btc. |
| src/coreclr/jit/codegenxarch.cpp | Implements codegen for GT_BIT_SET/CLEAR/INVERT using bts/btr/btc. |
| src/coreclr/jit/codegen.h | Declares genCodeForBitOp. |
| src/coreclr/jit/gtlist.h | Enables GT_BIT_* nodes for xarch (shared with riscv64). |
Copilot's findings
- Files reviewed: 13/13 changed files
- Comments generated: 7
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
C# defines 1 << y as masking the shift count modulo the operand width, and this JIT already models shl/shr as masked (LowerShift drops redundant & 31/ & 63). �zhi instead leaves the source unchanged when the index is >= width, so lowering AND(x, (1 << y) - 1) to �zhi(x, y) miscompiled the source pattern for out-of-range y (e.g. Bzhi_I(x, 32) returned x instead of 0). Mask the index to the operand width before �zhi so it reproduces the masked-shift result exactly; the extra �nd reg, imm is contained and the transform stays a net size/PerfScore win in SPMI (-3,425 bytes overall, all collections green). Also address related review feedback: rewrite the header comment to document the masked-index invariant, move TryLowerBitwiseOpToBitOp out of the FEATURE_HW_INTRINSICS region (bts/btr/btc are not hardware intrinsics), add a defensive assert in genCodeForBitOp documenting the LSRA delayFree invariant on the bit index, and expand the Bzhi test to cover indices past the operand width via an oracle that routes through an opaque masked shift. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The disasm checks used `{{[a-z]+}}` for register operands, which only matches
registers made purely of letters (eax/ecx/rax). When register allocation places
an operand in r8-r15 (e.g. `bts r8d, r10d`) the pattern fails to match and the
check spuriously fails; it passes today only when RA happens to pick a lettered
register. Widen the operand patterns to `{{[a-z0-9]+}}`.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…diate form The assert(targetReg != op2) added for the GT_BIT_* codegen is unsound: for x <op> (1 << x) op1 and op2 share an interval, so AddDelayFreeUses skips the delayFree and LSRA may place the destination on op2's register. That is safe -- op1 and op2 hold the same value, so the mov writes op2's own value back -- but it can't be expressed as a cheap codegen assert. JitStressRegs=0x800 reaches the aliased form and computes the correct result. Also widen the Set10 FileCheck to accept 1024 or 0x400, matching the file's other immediate patterns. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…x contract TryLowerAndOpToZeroHighBits documented that the shift count is never constant here (a constant 1 << Y folds to a constant mask during morph), but nothing enforced it. Like the un-canonicalized SUB form the function already guards against, a post-morph constant index could reach here and regress the optimal and reg, imm into mov reg, imm + bzhi, since bzhi has no immediate index form. Bail explicitly on a constant index. SPMI x64 confirms this is codegen-neutral: clean diff across all 10 collections (~2.6M contexts). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@MihuBot -nuget |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lowers several single-bit and bit-field idioms to dedicated x86 instructions on xarch:
x & ((1 << y) - 1)bzhi(reusesNI_AVX2_ZeroHighBits)x | (1 << y)bts(GT_BIT_SET)x & ~(1 << y)btr(GT_BIT_CLEAR)x ^ (1 << y)btc(GT_BIT_INVERT)(x >> y) & 1in== 0/!= 0bt(GT_BITTEST_EQ/GT_BITTEST_NE)Details
bzhiis recognized inLowerBinaryArithmeticand folded into the existing AVX2 scalar intrinsic, so containment and memory operands come for free. It matches both theADD(-1)andSUB(1)mask forms and only fires for a variable shift amount, since a constant amount folds to a plainandwith an immediate.bts/btr/btcreuse the existingGT_BIT_SET/GT_BIT_CLEAR/GT_BIT_INVERTopers (shared with riscv64) with xarch codegen, LSRA, and emitter support. Only thereg,regform is used: the value operand is kept in a register so the implicit masking of the bit index (mod the operand width) matches theshlsemantics, and because the memory-destination forms are slower. They likewise only fire for a variable bit index.btrecognizes(x >> y) & 1feeding a== 0/!= 0and emits the flag-producing bit test.bextrdeferred#81514also coversbextr, but constant-controlbextr((x >> c) & mask) is intentionally left out: it must materialize the control word and is multi-uop, soshr/sar+andin place is smaller and faster. SPMI confirmed it was a net regression (+7,671bytes) and it only wins when the source is a live-after local (saving a preservation copy) — which needs last-use info not available during lowering. The fully-variable(x >> y) & ((1 << z) - 1)case already lowers optimally toshrx + bzhi, so nothing is lost.A cost-aware follow-up prototyped the variable/mixed-control cases (the only ones not previously implemented) and measured them in isolation with SPMI asmdiffs over 2.6M contexts:
+61bytes, zero improvements, every diff a regression. Non-constant control does not avoid the materialization — it replaces the freeand reg, immmask with a multi-instruction packed-control build feeding a multi-uopbextr, andshrx/bzhialready provide the non-destructive form that wasbextr's only structural edge. Sobextris left out in all quadrants.Diffs
SPMI asmdiffs (x64 windows): overall
-3,874bytes, every collection improves in both size and PerfScore (coreclr_tests PerfScore-5.09%), no asserts, no replay failures. The only regressions are 6 methods at+1/+2bytes each (memory-RMWbtswhere the memory destination can't be contained without breaking the bit-index masking) — negligible against the wins.Contributes to #81514
Fixes #81512
Note
This PR description was generated with the assistance of GitHub Copilot.