Uh oh!
There was an error while loading. Please reload this page.
JIT: Transform arithmetic using distributive property - #126852
JIT: Transform arithmetic using distributive property#126852BoyBaykiller wants to merge 18 commits into
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| return tree; | ||
| } | ||
| if (((tree->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) != 0) || ((tree->gtFlags & GTF_ORDER_SIDEEFF) != 0)) |
There was a problem hiding this comment.
This could use the same optimization you are trying to apply😅... Probably handled by c++ though, so this is purely documentation ;-)
There was a problem hiding this comment.
Lol. A new level of self documenting code. I didn't even notice that, I copied this check from elsewhere. I guess this just goes to show how useful the optimization is. Unfortunately even with this PR it's still not handled : (
There was a problem hiding this comment.
Update: This now gets handled by calling this opt again after the "optimizeBools" phase where it simplifies e.g(A & 4) != 0 || (A & 8) != 0 into ((A & 4) | (A & 8)) != 0.
@EgorBo PTAL. Diffs are rather small, but nonetheless I believe it's a step in the right direction. I've written down some future work (which all improve diffs). I think the most interesting case for real-world code is arround bitwise ops. |
BoyBaykiller
commented
Apr 15, 2026
Regarding point 1. The issue is that this runs first: runtime/src/coreclr/jit/morph.cpp Lines 10665 to 10690 in e0fb1f9 Perhaps this should be moved to Lower. LLVM also does it in "X86 DAG->DAG Instruction Selection" and not "InstCombine" https://godbolt.org/z/jsPea1PhP |
Uh oh!
There was an error while loading. Please reload this page.
e92d4c5 to
8e06971CompareUh oh!
There was an error while loading. Please reload this page.
| auto isLeftDistributive = [](genTreeOps op1, genTreeOps op2) { | ||
| // op1 is left distributive over op2 iff: | ||
| // "A op1 (B op2 C)" <==> "(A op1 B) op2 (A op1 C)" |
There was a problem hiding this comment.
We already do something similar for GT_ADD somewhere, we should unify it with that
There was a problem hiding this comment.
I didn't find what you are refering to here
Uh oh!
There was an error while loading. Please reload this page.
* add OR and AND are 'distributive' over themselves
…implification like '(A & 4) != 0 || (A & 8) != 0'
Uh oh!
There was an error while loading. Please reload this page.
…rm-using-distributive-property
BoyBaykiller
commented
May 24, 2026
I've limited it to |
Basically so we always have 0 at the right-side:
1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'
Tthis currently helps optimizeBools, but I am also adding this with the
future in mind.
Here is one example:
```cs
static bool Example(int foo)
{
return ((foo & 4) == 4) || ((foo & 8) == 8);
}
```
```assembly
;; ------ BASE
G_M000_IG02: ;; offset=0x0000
test cl, 4
je SHORT G_M000_IG05
G_M000_IG03: ;; offset=0x0005
mov eax, 1
G_M000_IG04: ;; offset=0x000A
ret G_M000_IG05: ;; offset=0x000B
test cl, 8
setne al
movzx rax, al
G_M000_IG06: ;; offset=0x0014
ret ;; ------ DIFF
G_M33938_IG02: ;; offset=0x0000
mov eax, ecx
and eax, 4
and ecx, 8
or eax, ecx
setne al
movzx rax, al
```
Note: With #126852 this would then
get transformed into just `test 12`.…rm-using-distributive-property
JulieLeeMSFT
commented
Jun 29, 2026
@EgorBo, PTAL. |
EgorBo
commented
Jun 30, 2026
@MihuBot -nuget |
EgorBo
commented
Jul 1, 2026
MihaZupan
commented
Jul 1, 2026
Just testing error reporting changes @MihuBot -nuget |
MihuBot
commented
Jul 1, 2026
Warning 2 extra test assemblies failed to produce JIT diffs and were excluded from the diff analysis:
All failures happened only on the PR branch, which likely indicates a bug introduced by the PR (e.g. a JIT assert or crash while compiling these assemblies). See job details in MihuBot/runtime-utils#2058, triggered by #126852 (comment). |
Basically so we always have 0 at the right-side:
1. '(A & pow2) == pow2' -> '(A & pow2) != 0'
2. '(A & pow2) != pow2' -> '(A & pow2) == 0'
Tthis currently helps optimizeBools, but I am also adding this with the
future in mind.
Here is one example:
```cs
static bool Example(int foo)
{
return ((foo & 4) == 4) || ((foo & 8) == 8);
}
```
```assembly
;; ------ BASE
G_M000_IG02: ;; offset=0x0000
test cl, 4
je SHORT G_M000_IG05
G_M000_IG03: ;; offset=0x0005
mov eax, 1
G_M000_IG04: ;; offset=0x000A
ret G_M000_IG05: ;; offset=0x000B
test cl, 8
setne al
movzx rax, al
G_M000_IG06: ;; offset=0x0014
ret ;; ------ DIFF
G_M33938_IG02: ;; offset=0x0000
mov eax, ecx
and eax, 4
and ecx, 8
or eax, ecx
setne al
movzx rax, al
```
Note: With #126852 this would then
get transformed into just `test 12`.Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "ef82e04e0d0ec0a577f66e436b30a9adf3b47b77",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f32edc0223b18865778e8e4d335e31bf9a2933f0",
"last_reviewed_commit": "ef82e04e0d0ec0a577f66e436b30a9adf3b47b77",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f32edc0223b18865778e8e4d335e31bf9a2933f0",
"last_recorded_worker_run_id": "29679146097",
"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": "ef82e04e0d0ec0a577f66e436b30a9adf3b47b77",
"review_id": 4730527763
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Generalizes #126070 and makes progress on #93954 by teaching the JIT to simplify arithmetic via the distributive property, i.e. (A op1 B) op2 (A op1 C) => A op1 (B op2 C). This reduces instruction count in common patterns (e.g. (A*B)+(A*C) folding to A*(B+C), and bit-test patterns after optOptimizeBools), which is a worthwhile codegen improvement.
Approach: Adds gtFoldDistributiveArithmetic invoked from gtFoldExprBinary for GT_AND/GT_OR/GT_XOR/GT_ADD/GT_SUB trees. It uses an isLeftDistributive table (AND/OR/MUL over their duals) and, when both operands share the same outer op and their left children are matching locals, rebuilds the tree with the common factor extracted. It also re-runs gtFoldExpr on the newly-combined boolean fold result in optOptimizeBoolsUpdateTrees, and tidies an unrelated cmp->gtOp2 -> cmp->gtGetOp2() reference in morph.cpp.
Summary: The transform is conceptually sound for wrapping (unchecked) integer arithmetic, and the guarding on optimization level, overflow of the outer node, side effects, and integral type is a reasonable start. However there is one correctness concern worth resolving before merge: the overflow check only examines the outer tree, not the inner operands. For checked(A*B) + checked(A*C) the inner GT_MUL nodes carry GTF_OVERFLOW, yet the rebuilt A*(B+C) uses a plain unchecked GT_MUL, dropping the overflow checks and altering observable behavior (a missing OverflowException). See the inline comment on the guard. I'd recommend adding a targeted regression test covering the checked-arithmetic case (and confirming it is rejected) once the guard is tightened. A secondary, non-blocking note: the freshly-created newOp2 node does not get value numbers assigned even though gtFoldExpr may be reached post-VN via optOptimizeBools; only result receives SetVNsFromNode. Verify this is benign in that phase (constants typically fold away, but a non-constant B op C combination would leave an unnumbered node).
Detailed Findings
- Checked-arithmetic correctness (see inline comment on
gtFoldDistributiveArithmetic): outer-only overflow guard can drop inner checked-multiply/add/sub semantics. Recommend also bailing when the operands being merged havegtOverflowEx()/GTF_EXCEPTset. - VN coverage of
newOp2(non-blocking):SetVNsFromNode(tree)is applied toresultonly; the intermediatenewOp2 = gtFoldExpr(B op C)node has no VN. Since this path is also reachable fromoptOptimizeBoolsUpdateTreesafter value numbering, confirm an unnumbered non-constant intermediate cannot reach a consumer that requires valid VNs. - Scope observation (non-blocking): the transform requires the shared factor to be
OperIsAnyLocal. This is a safe, conservative choice (no duplicated side effects / re-evaluation), consistent with the PR description noting reassociation is not yet handled.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 115.6 AIC · ⌖ 11 AIC · ⊞ 10K
| { | ||
| assert(tree->OperIs(GT_AND, GT_OR, GT_XOR, GT_ADD, GT_SUB)); | ||
| if (opts.OptimizationDisabled() || tree->gtOverflowEx() || !varTypeIsIntegralOrI(tree)) |
There was a problem hiding this comment.
The overflow guard only inspects the outer tree, not the inner operands op1/op2. For an inner distributive op that can overflow-throw, this can silently drop checked semantics. Concretely, checked(A * B) + checked(A * C) has an unchecked outer GT_ADD but two inner GT_MUL nodes carrying GTF_OVERFLOW. isLeftDistributive(GT_MUL, GT_ADD) is true, so the transform rebuilds the tree as A * (B + C) via gtNewOperNode(GT_MUL, ...) with no overflow flag, discarding both checked multiplications. That changes observable behavior (a program that should throw OverflowException no longer does). For unchecked/wrapping arithmetic the distribution is correct mod 2^n, but the checked case is not. Consider also rejecting when op1/op2 (or their relevant children) have gtOverflowEx() set, or more conservatively bail when GTF_EXCEPT is present on the operands being merged.
There was a problem hiding this comment.
@BoyBaykiller that is a good point, this PR basically breaks this:
staticintF(inta,intb,intc)=>checked(a*b)+checked(a*c);With:
a = 2
b = 0x40000000
c = -0x40000000
There was a problem hiding this comment.
I am now bailing on GTF_EXCEPT
| { | ||
| GenTree* newOp1 = op1->gtGetOp1(); | ||
| GenTree* newOp2 = | ||
| gtFoldExpr(gtNewOperNode(tree->OperGet(), tree->TypeGet(), op1->gtGetOp2(), op2->gtGetOp2())); |
There was a problem hiding this comment.
seems like this gtNewOperNode is not marked as MORPHED
There was a problem hiding this comment.
added fgMorphTreeDone(newOp2);
…rm-using-distributive-property
* mark new op2 as fgMorphTreeDone
BoyBaykiller
commented
Jul 21, 2026
Failures fixed? @MihuBot -nuget |
Generalization of #126070
Progress towards #93954
Basically we weren't doing any simplification based on distributive property before. So transforming
((A op1 B) op2 (A op1 C)) => (A op1 (B op2 C)). And this adds some basic support. Examples:We still need something that changes order to enable this opt. So (A | B) | C becoming A | (B | C) in this case:
Or here, the
C * Aneed to be reversed.