JIT: dominating redundant branch opt#126587
Conversation
In some cases we can optimize a dominating branch, if its predicate is implied by a dominated branch. For example in ```if (x > 0) if (x > 1) S();``` `if (x > 0)` can be optimized away. Closes dotnet#126554
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@EgorBo PTAL A few hundred diffs; one common case is ;; ------ BASE
G_M16163_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
movzx rax, cx
movzx rcx, dx
cmp eax, ecx
je SHORT G_M16163_IG05
;; size=10 bbWeight=1 PerfScore 1.75
G_M16163_IG03: ; bbWeight=1.00, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
cmp ecx, eax
jge SHORT G_M16163_IG05
;; size=4 bbWeight=1.00 PerfScore 1.25
G_M16163_IG04: ; bbWeight=0.53, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, epilog, nogc
ret
;; size=1 bbWeight=0.53 PerfScore 0.53
G_M16163_IG05: ; bbWeight=0.47, gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
mov eax, ecx
;; size=2 bbWeight=0.47 PerfScore 0.12
G_M16163_IG06: ; bbWeight=0.47, epilog, nogc, extend
ret
;; ------ DIFF
G_M16163_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
movzx rax, dx
movzx rcx, cx
cmp eax, ecx
cmovl eax, ecx
;; size=11 bbWeight=1 PerfScore 1.00
G_M16163_IG03: ; bbWeight=1, epilog, nogc, extend
ret |
There was a problem hiding this comment.
Pull request overview
Adds a new redundant-branch optimization to the CoreCLR JIT that can eliminate an outer (dominating) conditional branch when an inner (dominated) conditional implies it, improving code quality for certain nested-if patterns (e.g., issue #126554).
Changes:
- Add
optRedundantDominatingBranchto identify and constant-fold dominatingBBJ_CONDbranches when implied by a dominated compare. - Introduce
BasicBlock::hasSideEffects()helper used to ensure it’s safe to speculatively execute dominated blocks. - Add a new JIT regression test covering dominating-branch elimination scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/redundantbranchopts.cpp | Implements the new “dominating redundant branch” optimization and wires it into the RBO phase. |
| src/coreclr/jit/compiler.h | Declares the new optimizer entrypoint. |
| src/coreclr/jit/block.h | Adds BasicBlock::hasSideEffects() helper used by the optimization. |
| src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.csproj | New test project for the regression coverage. |
| src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.cs | New functional test cases that exercise dominating-branch elimination patterns. |
| { | ||
| if (!block->KindIs(BBJ_COND)) |
There was a problem hiding this comment.
this check already exists at the call-site, so perhaps should be an assert?
EgorBo
left a comment
EgorBo
left a comment
There was a problem hiding this comment.
makes sense to me, would like to see the CI status/diffs
|
|
||
| if (!tree->OperIsCompare()) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
nit: in many places in JIT we copy this logic, we should probably either have a helper like "isCanonincalJtrue" or make it a hard rule how JTRUE's last statement should look like
There was a problem hiding this comment.
Good idea. I will do this as a no-diff follow-up.
|
|
||
| // Always preserve side effects in the dominating relop. | ||
| // | ||
| if ((domTree->gtFlags & GTF_SIDE_EFFECT) != 0) |
There was a problem hiding this comment.
maybe it could use gtWrapWithSideEffects here?
There was a problem hiding this comment.
I'll also do this as a no-diff follow up, since the same pattern exists in optRedundantBranch
… up, don't require always targeting the same successor
In some cases we can optimize a dominating branch, if its predicate is implied by a dominated branch. For example in
if (x > 0) if (x > 1) S();if (x > 0)can be optimized away.Closes #126554