JIT: Fix invalid IR and weights in if-conversion#125072
JIT: Fix invalid IR and weights in if-conversion#125072BoyBaykiller wants to merge 11 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
* format
There was a problem hiding this comment.
Pull request overview
This PR fixes CFG/IR correctness issues in CoreCLR JIT if-conversion, especially for return-based if/else patterns, and adjusts how unreachable then/else blocks are treated after conversion so the transformed IR no longer has a RETURN block with a successor.
Changes:
- Improve DEBUG dumping for if-conversion flows pre- and post-transformation.
- Update if-conversion CFG rewriting to correctly produce
BBJ_RETURNwhen converting return-based branches, and to detach then/else blocks from the start block. - Adjust weights for then/else blocks after their incoming flow is removed.
* update comments
a74nh
left a comment
There was a problem hiding this comment.
I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.
* don't dump m_finalBlock
a74nh
left a comment
There was a problem hiding this comment.
Given removing the BB blocks isn't required, I'm happy with this.
|
Now, that if-conversion no longer produces flow through empty blocks I think we can make the following assumptions: assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}I've run Previously this wasn't true. There could be empty blocks after Of course if previous phases produce empty blocks inside Then/Else then we'd miss cases, but at least currently this doesn't seem to happen. I want to simplify the code using these assumptions in a follow-up PR. |
|
I've applied jakobs suggestion to remove all Then/Else blocks. I also got rid of the the NOP after the SELECT: After if conversion
\------------ BB01 [0000] [000..005) -> BB04(1) (always), preds={} succs={BB04}
***** BB01 [0000]
STMT00001 ( 0x002[E--] ... 0x003 )
N001 ( 0, 0) [000005] -----+----- * NOP void
***** BB01 [0000]
STMT00005 ( 0x005[E--] ... 0x006 )
N008 ( 12, 15) [000012] DA---+----- * STORE_LCL_VAR int V01 loc0 d:3 $VN.Void
N007 ( 8, 12) [000018] ----------- \--* SELECT int
N004 ( 5, 7) [000004] J----+-N--- +--* EQ int $101
N002 ( 3, 4) [000013] -----+----- | +--* CAST int <- ubyte <- int $100
N001 ( 2, 2) [000002] -----+----- | | \--* LCL_VAR int V00 arg0 u:1 (last use) $80
N003 ( 1, 2) [000003] -----+----- | \--* CNS_INT int 0 $40
N005 ( 1, 2) [000006] -----+----- +--* CNS_INT int 9 $43
N006 ( 1, 2) [000011] -----+----- \--* CNS_INT int 2 $44
- ***** BB01 [0000]
- STMT00002 ( 0x009[E--] ... 0x00B )
- N001 ( 0, 0) [000007] -----+----- * NOP void This is simply done by no longer appending the Else block. That means |
|
I now replace the JTRUE statement with the USE(SELECT(...)) instead of appending it and bashing JTRUE to NOP. This means all NOPs are gone. Ready for an other review. |
|
With the latest version, the IR dump on my test cases look much better. Everything that was optimised becomes a single block with no junk lying around. Plus the removal of the statements and basicblocks automatically get lines added in the dump. |
jakobbotsch
left a comment
There was a problem hiding this comment.
LGTM! Thanks for working on this.
Fix #124942.
Small diffs are expected because:
It allows if-conversion to recognize more cases.
For example https://godbolt.org/z/Pdj898qTT currently still produces one jmp. But with this change it's eliminated, as the search now knows when there actually is a return block (previously it wasn't marked as such).
The block weights changed. For example previously
produced blocks with
bbWeight = 0.5, now it's 1.0.