JIT: Fix invalid IR and weights in if-conversion

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure

[BoyBaykiller]

Fix#124942.

Small diffs are expected because:

1. 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).

2. The block weights changed. For example previously

staticintInvalidIR(boolcond){if(cond){return2;}// or else {}return4;}

produced blocks with bbWeight = 0.5, now it's 1.0.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

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_RETURN when 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.

[a74nh]

I tested this with some if/then, if/then/else, if/then/else/return cases. I'm happy the IR now looks correct afterwards.

[a74nh]

Given removing the BB blocks isn't required, I'm happy with this.

[BoyBaykiller]

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 python .\src\coreclr\scripts\superpmi.py replay on this and haven't seen any failures. This would allow us to
simplify some parts, most noticeably FindFlow().

Previously this wasn't true. There could be empty blocks after m_{then/else}Operation.Block before reaching m_finalBlock. E.g System.Net.SocketAddress:GetMaximumAddressSize() or 3039 in aspnet2.run.windows.x64.checked.mch

Conditionally executing BB03 and BB05 inside BB02
------------ BB02 [0001] [004..008) -> BB05(0.5),BB03(0.5) (cond), preds={BB01} succs={BB03,BB05}
***** BB02 [0001]
STMT00003 ( 0x004[E--] ... 0x006 )
N004 ( 5, 5) [000011] -----+----- * JTRUE void $VN.Void
N003 ( 3, 3) [000010] J----+-N--- \--* EQ int $101
N001 ( 1, 1) [000008] -----+----- +--* LCL_VAR int V00 arg0 u:1 $80
N002 ( 1, 1) [000009] -----+----- \--* CNS_INT int 2 $44
------------ BB03 [0002] [008..00D) -> BB06(1) (always), preds={BB02} succs={BB06}
***** BB03 [0002]
STMT00005 ( 0x008[E--] ... 0x00B )
N001 ( 0, 0) [000017] -----+----- * NOP void ***** BB03 [0002]
STMT00007 ( 0x027[E--] ... 0x02C )
N007 ( 6, 9) [000021] DA---+----- * STORE_LCL_VAR int V01 loc0 d:4 $VN.Void
N006 ( 6, 9) [000028] ----------- \--* SELECT int N003 ( 3, 3) [000016] J----+-N--- +--* EQ int $102
N001 ( 1, 1) [000014] -----+----- | +--* LCL_VAR int V00 arg0 u:1 (last use) $80
N002 ( 1, 1) [000015] -----+----- | \--* CNS_INT int 23 $46
N004 ( 1, 1) [000018] -----+----- +--* CNS_INT int 28 $47
N005 ( 1, 4) [000020] -----+----- \--* CNS_INT int 128 $48
***** BB03 [0002]
STMT00006 ( 0x017[E--] ... 0x01C )
N001 ( 0, 0) [000019] -----+----- * NOP void ------------ BB06 [0005] [017..01F) -> BB09(1) (always), preds={BB03} succs={BB09}
------------ BB05 [0004] [00F..017) -> BB09(1) (always), preds={BB02} succs={BB09}
***** BB05 [0004]
STMT00004 ( 0x00F[E--] ... 0x014 )
N002 ( 1, 3) [000013] DA---+----- * STORE_LCL_VAR int V01 loc0 d:2 $VN.Void
N001 ( 1, 1) [000012] -----+----- \--* CNS_INT int 16 $45

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.

[BoyBaykiller]

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 IfConvertJoinStmts is only called once. And since we really only want to append m_thenOperation.stmt and not the entire block (it might have NOPs), I've decided to remove that function and append m_thenOperation.stmt inline.

[BoyBaykiller]

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.

[a74nh]

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]

LGTM! Thanks for working on this.

[jakobbotsch]

/ba-g Timeouts, android/tvOS failures and 404 superpmi-diffs failure