Clear some stale GTF_ORDER_SIDEEFF flags#130007
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adjusts JIT tree side-effect flag maintenance by attempting to clear stale GTF_ORDER_SIDEEFF bits during Compiler::gtUpdateStmtSideEffects, to unblock downstream optimizations that are inhibited by incorrectly-propagated ordering constraints.
Changes:
- Add logic in
gtUpdateStmtSideEffectsto clearGTF_ORDER_SIDEEFFon nodes that don’t support ordering side effects when none of their operands still carry the flag.
|
link to diffs https://dev.azure.com/dnceng-public/public/_build/results?buildId=1486826&view=ms.vss-build-web.run-extensions-tab Example (more ccmp in a method similar to the motivating example) -4 (-7.14%) : AggregateSymbol:IsRefType():bool:this (FullOpts)@@ -22,30 +22,27 @@ G_M9025_IG02: ; bbWeight=1, gcrefRegs=0001 {x0}, byrefRegs=0000 {}, byref
; gcrRegs +[x0]
ldr w1, [x0, #0x90]
cmp w1, #1
- beq G_M9025_IG04
+ bne G_M9025_IG05
;; size=12 bbWeight=1 PerfScore 4.50
-G_M9025_IG03: ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
+G_M9025_IG03: ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
; gcrRegs -[x0]
- cmp w1, #3
- bne G_M9025_IG06
- ;; size=8 bbWeight=0.50 PerfScore 0.75
-G_M9025_IG04: ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
mov w0, #1
;; size=4 bbWeight=0.50 PerfScore 0.25
-G_M9025_IG05: ; bbWeight=0.50, epilog, nogc, extend
+G_M9025_IG04: ; bbWeight=0.50, epilog, nogc, extend
ldp fp, lr, [sp], #0x10
ret lr
;; size=8 bbWeight=0.50 PerfScore 1.00
-G_M9025_IG06: ; bbWeight=0.50, gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
- cmp w1, #2
+G_M9025_IG05: ; bbWeight=0.50, gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
+ cmp w1, #3
+ ccmp w1, #2, z, ne
cset x0, eq
- ;; size=8 bbWeight=0.50 PerfScore 0.50
-G_M9025_IG07: ; bbWeight=0.50, epilog, nogc, extend
+ ;; size=12 bbWeight=0.50 PerfScore 0.75
+G_M9025_IG06: ; bbWeight=0.50, epilog, nogc, extend
ldp fp, lr, [sp], #0x10
ret lr
;; size=8 bbWeight=0.50 PerfScore 1.00
-; Total bytes of code 56, prolog size 8, PerfScore 9.50, instruction count 14, allocated bytes for code 56 (MethodHash=4337dcbe) for method Microsoft.CSharp.RuntimeBinder.Semantics.AggregateSymbol:IsRefType():bool:this (FullOpts)
+; Total bytes of code 52, prolog size 8, PerfScore 9.00, instruction count 13, allocated bytes for code 52 (MethodHash=4337dcbe) for method Microsoft.CSharp.RuntimeBinder.Semantics.AggregateSymbol:IsRefType():bool:this (FullOpts)
; ============================================================ |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
I assume we end up in this state because we fail to clear the flag when we change the oper?
Should we add an IR check that we never see GTF_ORDER_SIDEEFF on a node where it isn't expected?
I agree it would be nice to make |
@jakobbotsch + @AndyAyersMS sequence of steps here was, null check gets removed on an IND so it + the parent EQ is marked SIDEEFFECT: IND gets CSE'd with a local, but EQ retains the bit (the gtUpdateStmtSideEffects method im changing here is called immediately after the CSE) |
|
Opened #130150 to track the bad diff with the ballooned perfscore |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a715ad358df9038ffe2e22d2ebc83cd4358993ef",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "fe5e47348f86013bbe8f3041e56f5208cd632e53",
"last_reviewed_commit": "a715ad358df9038ffe2e22d2ebc83cd4358993ef",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "fe5e47348f86013bbe8f3041e56f5208cd632e53",
"last_recorded_worker_run_id": "29679159636",
"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": "a715ad358df9038ffe2e22d2ebc83cd4358993ef",
"review_id": 4730522596
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real: stale GTF_ORDER_SIDEEFF bits left on nodes whose oper does not support an ordering side effect block downstream optimizations (optimize-bools / if-conversion), as shown by the o.GetType() == typeof(...) OR-combining example. Clearing conservatively-set ordering bits is a legitimate cleanup.
Approach: The fix is placed correctly in the gtUpdateStmtSideEffects pre/post-order walker. Clearing the bit in PreOrderVisit when !OperSupportsOrderingSideEffect() is safe because PostOrderVisit re-propagates GTF_ALL_EFFECT (which includes GTF_ORDER_SIDEEFF) from children to the parent, so any node that legitimately inherits the flag from an operand gets it re-set. OperSupportsOrderingSideEffect() returns true for TYP_BYREF, so byref-forming nodes are not incorrectly stripped. This mirrors the existing canonicalization in OperEffects (gentree.cpp ~L8844), keeping the two paths consistent.
Summary: ✅ LGTM. The change is minimal, correct, and internally consistent with existing GTF_ORDER_SIDEEFF handling. Per the JIT folder guidance, a dedicated regression test is not required for this codegen-quality change validated by differential (SuperPMI) diffs; the author already notes reviewing the resulting good/bad diffs. Worth tracking in the PR discussion (as AndyAyersMS raised): the ideal long-term fix is to clear the flag at the point the oper is changed, and/or add an IR-consistency check asserting GTF_ORDER_SIDEEFF never appears on an unsupported node without a child carrying it. Those are reasonable follow-ups rather than blockers.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 66.2 AIC · ⌖ 14.5 AIC · ⊞ 10K
a715ad3 to
ef032b0
Compare
|
@AndyAyersMS + @jakobbotsch I added a Debug check for this. I also needed flag updates in fgMorphArgs and gtUpdateNodeOperSideEffects |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/coreclr/jit/gentree.cpp:11855
- The
gtUpdateNodeSideEffectsdoc comment is now out of date: it claims only EXCEPT/ASG/CALL are updated, butgtUpdateNodeOperSideEffectsnow also clears staleGTF_ORDER_SIDEEFF, andgtUpdateNodeSideEffectspropagatesGTF_ALL_EFFECTfrom children (includingGTF_ORDER_SIDEEFF). Consider updating the comment (and fix the "side efects" typo) so it reflects current behavior.
src/coreclr/jit/fgdiagnostic.cpp:3652 - Minor typo/clarity issue in the comment describing when
GTF_ORDER_SIDEEFFis treated as "extra": the current text says "set set" and implies an explicit child check here, but the child condition is already implied byactualFlags & ~expectedFlagsin this branch. Consider tightening the wording.
// GTF_ORDER_SIDEEFF is stale if set set on a node whose oper does not support it,
// and whose children do not have it set
if (tree->OperSupportsOrderingSideEffect())
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/fgdiagnostic.cpp:3652
- The comment describing when GTF_ORDER_SIDEEFF is considered stale is misleading: this helper doesn't directly inspect children. Instead, because expectedFlags already unions in child effects, an extra GTF_ORDER_SIDEEFF here indicates staleness only when the oper doesn't support it and it's not coming from children (i.e., not in expectedFlags). Updating the comment will help future maintainers reason about the mask logic.
// GTF_ORDER_SIDEEFF is stale if set on a node whose oper does not support it,
// and whose children do not have it set
if (tree->OperSupportsOrderingSideEffect())
Clear stale `GTF_ORDER_SIDEEFF` flags to enable more if-conversion and optimize bools. Motivating example was `o.GetType() == typeof(int) || o.GetType() == typeof(uint) || o.GetType() == typeof(long)` - we couldn't combine into `OR` because leftover side effect bit in one of the blocks. If an op doesn't itself support side effects, and none of node's children have the bit, then the node may have the bit cleared. Diffs show more optimize bools+if conversion taking place. One of the bad diffs was block layout changing and causing LSRA to spill slightly different. Another bad diff exposed an issue where edge-likelihoods weren't recalculated properly after optimize bools, and caused perfscore to balloon. I'm working on a fix for that seperately.
Clear stale
GTF_ORDER_SIDEEFFflags to enable more if-conversion and optimize bools.Motivating example was
o.GetType() == typeof(int) || o.GetType() == typeof(uint) || o.GetType() == typeof(long)- we couldn't combine intoORbecause leftover side effect bit in one of the blocks.If an op doesn't itself support side effects, and none of node's children have the bit, then the node may have the bit cleared.
Diffs show more optimize bools+if conversion taking place.
One of the bad diffs was block layout changing and causing LSRA to spill slightly different.
Another bad diff exposed an issue where edge-likelihoods weren't recalculated properly after optimize bools, and caused perfscore to balloon. I'm working on a fix for that seperately.