Uh oh!
There was an error while loading. Please reload this page.
arm64: Improve predicate instruction usage - #129506
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
dhartglassMSFT
commented
Jun 24, 2026
a74nh
commented
Jun 26, 2026
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "35eb3b6ab6b6b2f3a184e525eaf24e704c2c926f",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "1af2b9fda524303a30c5d14ac47bbf5510e06102",
"last_reviewed_commit": "35eb3b6ab6b6b2f3a184e525eaf24e704c2c926f",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "1af2b9fda524303a30c5d14ac47bbf5510e06102",
"last_recorded_worker_run_id": "29677704668",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "35eb3b6ab6b6b2f3a184e525eaf24e704c2c926f",
"review_id": 4730527203
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The motivation is real and well-evidenced. On SVE, boolean masks are naturally predicates, but the JIT currently materializes predicate results into vectors (mov z, p/z, #1) and re-compares them to zero before doing logical ops, then compares back. Recovering the predicate form lets the JIT emit and/bic/orr/eor/sel/zip1 directly on predicate registers and reuse the first predicate as the governing predicate, removing redundant ptrue/mov sequences. The PR's own SPMI diffs show real code-size and allocation wins.
Approach: The approach is reasonable and consistent with existing patterns: recognize mask-shaped operands (ConvertMaskToVector, real masks, vector zero, and constant 0/1 vectors) during gtFoldExprHWIntrinsic and in arm64 lowering, and fold to the _Predicates intrinsic variants. Importing Sve.BitwiseClear as AND(NOT(...)) and recovering bic in lowering mirrors the established AdvSimd handling. The codegen using op1Reg as the governing predicate matches the documented and Pd, Pn/z, Pn, Pm semantics.
Summary: TryLowerSvePredicateBitwiseClear ends with an unconditional return true; in its fall-through path. Since the helper now runs for every arm64 vector GT_AND, a true result when no predicate pattern matched causes the caller to break and skip the pre-existing AdvSimd ~op1 & op2 → BitwiseClear (AndNot) folding for ordinary vector ANDs. This is a functional regression outside the intended SVE-predicate scope and is not covered by the added SVE-only tests. See the inline comment for details.
Detailed Findings
❌ Correctness — Unconditional return true; disables AdvSimd AndNot folding
Flagged inline at src/coreclr/jit/lowerarmarch.cpp fall-through return. The final return true; should be return false; so that non-predicate vector ANDs fall through to the existing NI_AdvSimd_BitwiseClear/AndNot recognition in LowerHWIntrinsic. As written, that established optimization is bypassed for all arm64 vector GT_AND whose operands are not mask-shaped.
⚠️ Test coverage — No coverage for the non-predicate AndNot path
The added tests in PredicateInstructions.cs exercise only the new SVE-predicate folds (all inputs mask-shaped). None exercise an ordinary vector ~a & b / Vector*.AndNot on arm64, which is exactly the path broken by the finding above. A regression test covering that AdvSimd AndNot lowering would prevent this class of regression.
✅ Codegen and fold logic — Predicate variants look correct
The NI_Sve_And_Predicates/NI_Sve_BitwiseClear_Predicates codegen reusing op1Reg as the governing predicate matches the SVE semantics in the PR description, and the 0/1-vector-to-mask recognition in gtFoldExprHWIntrinsic is guarded to arm64 + masked HW intrinsics and rejects non-0/1 constants. TryLowerSveConvertVectorToMask correctly only elides the conversion when the source is already a mask.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 141.4 AIC · ⌖ 10.7 AIC · ⊞ 10K
| return true; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This fall-through return true; looks like a bug. When none of the three predicate patterns match (the common case for ordinary vector GT_AND), the function still returns true. The caller does:
if ((oper == GT_AND) && TryLowerSvePredicateBitwiseClear(node, &intrinsicId, &oper))
{
break;
}so a true result breaks out of the GT_AND/GT_OR case and skips the pre-existing AdvSimd folding that turns ~op1 & op2 into NI_AdvSimd_BitwiseClear (AndNot). Because this helper is now invoked for every arm64 vector GT_AND, that established AndNot recognition is effectively disabled for all non-predicate vector ANDs whenever the operands are not mask-shaped — a functional regression well beyond the intended SVE-predicate scope.
This should almost certainly return false; here so the caller falls through to the existing AdvSimd transform. (A regression test exercising e.g. Vector128.AndNot / ~a & b on arm64 would catch this; the current SVE-only tests do not.)
There was a problem hiding this comment.
I've fixed the return true issue.
dhartglassMSFT
commented
Jul 22, 2026
@jonathandavies-arm I'll take a look here now that #128326 has gone in, though it looks like there's merge conflicts |
…e-instruction-usage # Conflicts: # src/coreclr/jit/hwintrinsicarm64.cpp # src/coreclr/jit/lowerarmarch.cpp # src/tests/JIT/opt/SVE/PredicateInstructions.cs
dhartglassMSFT
commented
Jul 27, 2026
Failures in conditionalselect tests |
jonathandavies-arm
commented
Jul 28, 2026
This should be fixed now. I'll keep an eye on the tests. |
dhartglassMSFT
commented
Aug 3, 2026
There's a merge conflict, friendly ping @jonathandavies-arm |
…e-instruction-usage # Conflicts: # src/coreclr/jit/lowerarmarch.cpp
There was a problem hiding this comment.
Pull request overview
Improves ARM64 SVE predicate-codegen by recognizing more mask-shaped operands (including constant 0/1 vectors), enabling additional folding to predicate forms, and tightening codegen/containment for certain predicate operations. This is primarily a JIT/codegen optimization backed by targeted SVE JIT tests.
Changes:
- Teach folding/lowering to treat constant 0/1 vectors (and some inverted forms) as mask-shaped so more SVE ops fold to predicate variants.
- Add lowering/codegen support to emit more efficient predicate
and/bicforms (avoiding redundantptrue) and remove redundant vector↔mask conversions. - Extend the SVE predicate instruction test coverage to exercise the new folding/lowering cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/opt/SVE/PredicateInstructions.cs | Adds new SVE test methods/assertions covering 0/1 constant “mask-shaped” folding and predicate bic lowering. |
| src/coreclr/jit/lowerarmarch.cpp | Adds SVE mask-operand recognition/conversion helpers, lowers mask-shaped AND/NOT into predicate BIC, removes redundant ConvertVectorToMask, and refines embedded-mask wrapping/containment behavior. |
| src/coreclr/jit/lower.h | Declares new SVE-lowering helpers under FEATURE_MASKED_HW_INTRINSICS. |
| src/coreclr/jit/hwintrinsiccodegenarm64.cpp | Updates codegen for predicate and/bic to use the first predicate as the governing predicate. |
| src/coreclr/jit/hwintrinsicarm64.cpp | Adjusts import for NI_Sve_BitwiseClear to decompose to NOT+AND for later SVE predicate lowering. |
| src/coreclr/jit/gentree.cpp | Extends HWIntrinsic folding to treat 0/1 constant vectors as mask-shaped for all-mask SVE variants (including ConditionalSelect). |
Uh oh!
There was an error while loading. Please reload this page.
Change-Id: I8398ba28db6207ec720b1a31e2d8c3bc3a367142
dhartglassMSFT
commented
Aug 20, 2026
Unfortunately looks like there's still a test failure |
Change-Id: I92c28f208bd65690604d472b8546b720605cdd5b
There was a problem hiding this comment.
nit, move block comment down next to gtFoldExprHWIntrinsic's definition
| canFold &= tree->Op(i)->OperIsConvertMaskToVector(); | ||
| // Fold SVE intrinsics such as And/Or/Xor/ZipLow to their | ||
| // all-mask variants when every operand is mask-shaped. | ||
| canFold &= IsAllMaskVariantOperand(tree->Op(i), simdBaseType); |
There was a problem hiding this comment.
I think the transformation for this is incorrect if CreateBreakAfterMask is NOT here
Sve.And(Sve.CompareGreaterThan(a, b), Vector.Create<short>(1))
Should produce a simd vector with lanes either zero or one. But now the morph transform activates this and wraps it in "ConvertMaskToVector", causing result lanes to either be zero or allbitsset.
| // in lowering instead so decompose into the individual operations | ||
| // on import | ||
| op2 = gtNewSimdUnOpNode(GT_NOT, retType, op2, simdBaseType, simdSize); |
There was a problem hiding this comment.
It's unclear to me why we no longer fold here, can you explain?
(Also NIT but if possible probably better to leave these combined and fold only if intrinsic==NI_AdvSimd_BitwiseClear just to cut down code duplication)
There was a problem hiding this comment.
We don’t fold the NOT in the SVE case so that lowering can recognize mask shaped operands and emit the predicate BIC form.
| // IsSveMaskOperand: | ||
| // Return true if "op" can be represented directly as an SVE predicate. | ||
| // | ||
| static bool IsSveMaskOperand(GenTree* op) |
There was a problem hiding this comment.
This should be combined with IsAllMaskVariantOperand from gentree
There was a problem hiding this comment.
I've combined them
| node->ResetHWIntrinsicId(NI_Sve_BitwiseClear_Predicates, op1, op2); | ||
| node->gtType = TYP_MASK; | ||
| *intrinsicId = NI_Sve_BitwiseClear_Predicates; |
There was a problem hiding this comment.
The following code should trigger a INS_invalid assert, trying to do codegen for BitwiseClear_Predicates with floating point basetype:
[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector<float> ReproFloat(Vector<float> left, Vector<float> right)
{
Vector<float> firstMask = Sve.CompareLessThan(left, right);
Vector<float> secondMask = Sve.ZipLow(
Sve.CompareGreaterThan(left, right),
Sve.CompareEqual(left, right));
return firstMask & ~secondMask;
}
Maybe the hw intrinsic list table just needs a new entries for BitwiseClear_Predicates for fp type, or just reject floats if they're not allowed
There was a problem hiding this comment.
I added the floating point mappings to the intrinsics.
SVE Predicate Instructions asm diffs
Branch:
upstream/sve/predicate-instruction-usageCollection / JIT / output paths
Collection
artifacts/asmdiff/custom_mch/sve_predicate_instructions.mchartifacts/tests/coreclr/linux.arm64.Checked/JIT/opt/SVE/PredicateInstructions/PredicateInstructions.dllJITs
artifacts/asmdiff/builds/5656cc9c/core_root/libclrjit.soartifacts/bin/coreclr/linux.arm64.Checked/libclrjit.soOutput
artifacts/spmi/asm.sve_predicate_instructions/base/artifacts/spmi/asm.sve_predicate_instructions/diff/artifacts/spmi/superpmi.2.logSummary
3 asm diffs were found, all in
PredicateInstructions.AndMask(Vector<short>, Vector<short>)BitwiseClearMask(Vector<short>, Vector<short>)TransposeEvenAndMask(Vector<short>, Vector<short>, Vector<short>)Notable diffs
Context 10:
AndMaskRemoved a redundant
ptrueand now uses the first predicate as the governing predicate forand:Context 11:
BitwiseClearMaskUses predicate
bicdirectly instead of materializing predicate results into vectors and doing vectorbic:Context 16:
TransposeEvenAndMaskRemoves one
ptrue p2.h; total code size and PerfScore are unchanged: