JIT: Simplify if-conversion by BoyBaykiller · Pull Request #125347 · dotnet/runtime

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller · 2026-03-09T20:37:34Z

Zero-diff change that simplifies if-conversion based on the assumption that there are no empty blocks inside the Then and Else case. See: #125072 (comment)

dotnet-policy-service · 2026-03-09T20:38:31Z

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

* simplify IfConvertCheckStmts * small things

Copilot

Pull request overview

This PR simplifies the CoreCLR JIT if-conversion implementation by collapsing the prior multi-block flow search into a single, direct-flow check, under the assumption that Then/Else paths do not contain empty/chained blocks.

Changes:

Replace the prior bounded “walk” of Then/Else chains with a single IfConvertCheckFlow() check.
Simplify statement validation to operate on a single block (ignoring NOPs) rather than scanning a block chain to m_finalBlock.
Simplify debug dumping and block removal to target only the identified Then/Else blocks.

BoyBaykiller · 2026-03-12T21:57:37Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:

assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

Meaning no flow through empty blocks inside Then/Else cases.
So CheckFlow becomes just https://github.com/BoyBaykiller/runtime/blob/5c1616c5d263ec7ef07a1da4b8bc8224cdf3b97c/src/coreclr/jit/ifconversion.cpp#L73-L101. No diffs. I think cases where previous phases produce empty blocks aren't amendable to if-conversion anyway.

I simplified:

CheckFlow
CheckStmts
Dump
removeBlocks

a74nh · 2026-03-13T09:38:17Z

@a74nh @jakobbotsch What do you think? I guess the main question is whether this is a reasonable assumption to make in the first place:
assert(m_startBlock->GetFalseTarget()->GetUniqueSucc() == m_finalBlock);
if (m_doElseConversion)
{
assert(m_startBlock->GetTrueTarget()->GetUniqueSucc() == m_finalBlock);
}

The original assumption was that the graph potentially could contain multiple blocks with a straight line flow between them. What could give that scenario?

reaching a max number of statements in a block? I'm not sure if there even is a limit
if a previous optimisation left the graph that way (for example, if conversion prior to your fix). But that's for the previous optimisation to be fixed up.
Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.
if (x && y && z) isn't quite the same scenario, but that will already be optimised by optimise bools into CCMPs that can then be if converted.

No diffs

Even if it's not a 100% correct assumption, sounds like we're still catching everything of relevance with your change.

BoyBaykiller · 2026-03-14T05:11:38Z

Nested if then, eg if (x) { if(y) {...} else {...} } else { ... }. But if conversion can't optimise that scenario anyway.

Do you mean something like this. Because it does get recognized. We just bail on profitability for the outer one:
Skipping if-conversion that will evaluate RHS unconditionally at costs 9,1 .

inta74nh(boolx,booly){if(x){if(y){return1;}else{return2;}}else{return3;}}

moveax,3movecx,2movr10d,1testr8b,r8b cmovne ecx,r10dtestdl,dl cmovne eax,ecx ;; size=28 bbWeight=1 PerfScore 1.75G_M25408_IG03: ;; offset=0x001Cret

Anyway so yeah, while there will still be Then/Else cases with multiple blocks that have linear flow this PR says: "Such cases won't be amendable to if-conversion anyway so let's bail in CheckFlow()".
Previously such cases would have been considered valid flow only to bail out in CheckStmts immediately after (with #125072).

* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

BoyBaykiller · 2026-03-23T19:37:28Z

@a74nh does everything look ok now or are there other changes you would like me to do?

a74nh

I'm happy with this now.
Although, looks like formatting needs fixing.

jakobbotsch · 2026-04-08T11:30:24Z

@BoyBaykiller Can you address the formatting issue?

…to simplify-if-conv

jakobbotsch · 2026-04-09T14:25:43Z

@BoyBaykiller The jit-format job uploads a patch file when the formatting fails that you can apply locally to fix it. You can also run jit-format locally before you even commit: https://github.com/dotnet/jitutils/blob/main/doc/formatting.md

BoyBaykiller · 2026-04-10T03:28:12Z

Thanks. I'll know that for the future. In this case vscode formatting did the job

jakobbotsch

Thanks!

A Fuzzlyn-generated repro hits `assert(!block->HasFlag(BBF_DONT_REMOVE))` during the JIT's *If conversion* phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified `IfConvertCheckFlow` and inadvertently dropped the `BasicBlock::sameEHRegion` check previously performed by `IfConvertCheckInnerBlockFlow`. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it. ## Changes - **`src/coreclr/jit/ifconversion.cpp`** — In `OptIfConversionDsc::IfConvertCheckFlow`, bail when `falseBb` (Then) or `trueBb` (Else, when present) is not in the same EH region as `m_startBlock`. After merging `origin/main`, the Else guard uses the new `HasElseBlock()` helper (which replaced the removed `m_doElseConversion` member). - **`src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs`** — Regression test using the original repro. The test source is added to the shared merged `src/tests/JIT/Regression/Regression_ro_2.csproj` (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed. ```cpp if (falseBb->GetUniquePred(m_compiler) == nullptr) { return false; } // The Then/Else blocks will be removed by if-conversion, so they must be in the same // EH region as m_startBlock. Otherwise they may be the start of a try/handler region // (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state. if (!BasicBlock::sameEHRegion(falseBb, m_startBlock)) { return false; } m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb; if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock)) { return false; } ``` Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (`global::Runtime_127446.Runtime_127446.TestEntryPoint()` → Passed) after merging the latest `origin/main`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com>

BoyBaykiller changed the title ~~JIT: Simplify if conv~~JIT: Simplify if-conversionMar 9, 2026

github-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 9, 2026

dotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Mar 9, 2026

build-analysisBot mentioned this pull request Mar 9, 2026
Build error: ilc exited with code 57005 #124976
Closed

BoyBaykiller added 3 commits March 11, 2026 11:07

* simplify and rename FindFlow()
604b77b
* simplify IfConvertCheckStmts * small things

* try fix CI 'block != nullptr'
3450c64

* add and check assert(m_doElseConversion) in CI
5c1616c

BoyBaykiller force-pushed the simplify-if-conv branch from 9792586 to 5c1616cCompare March 11, 2026 10:07

This was referenced Mar 11, 2026
The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008
Open
[android-arm64] The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#6408
Closed

BoyBaykiller marked this pull request as ready for review March 11, 2026 18:05

EgorBo requested a review from Copilot March 11, 2026 18:15

Copilotstarted reviewing on behalf of EgorBo March 11, 2026 18:16 View session

CopilotAI reviewed Mar 11, 2026
View reviewed changes

Comment threadsrc/coreclr/jit/ifconversion.cpp

build-analysisBot mentioned this pull request Mar 12, 2026
MsQuic fails with QUIC_STATUS_OUT_OF_MEMORY on AzureLinux #123216
Closed

* plural to singluar in comment
43ed699

a74nh reviewed Mar 16, 2026
View reviewed changes

Comment threadsrc/coreclr/jit/ifconversion.cpp Outdated

a74nh reviewed Mar 16, 2026
View reviewed changes

Comment threadsrc/coreclr/jit/ifconversion.cpp

a74nh reviewed Mar 16, 2026
View reviewed changes

Comment threadsrc/coreclr/jit/ifconversion.cpp Outdated

* update IR in comment
44dcf06
* add 'assert(m_cond->OperIsCompare())' * combine IfConvertCheckFlow + IfConvertCheckStmts into new IfConvertCheck * get m_mainOper from m_thenOperation instead of flow check

This was referenced Mar 17, 2026
slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883
Open
XHarness package install failure on iOS due to devicectl NSPOSIXErrorDomain error 49 #123796
Open

BoyBaykiller mentioned this pull request Mar 18, 2026
Assert failure: Assertion failed '!block->HasFlag(BBF_DONT_REMOVE)' in 'Program:TestEntryPoint():int' #125600
Closed

a74nh approved these changes Mar 24, 2026
View reviewed changes

jakobbotsch self-requested a review March 24, 2026 10:01

Merge branch 'main' into simplify-if-conv
fa530ab

build-analysisBot mentioned this pull request Apr 8, 2026
System.Net.NameResolution.Tests DNS failures: Name or service not known #126641
Closed

BoyBaykillerand others added 3 commits April 8, 2026 22:22

* see if CTRL+K, CTRL+F in vscode fixes formatting failure
4ca15a0

Merge branch 'simplify-if-conv' of github.com:BoyBaykiller/runtime in…
febc824
…to simplify-if-conv

Merge branch 'dotnet:main' into simplify-if-conv
f9d0026

BoyBaykiller closed this Apr 9, 2026

BoyBaykiller reopened this Apr 9, 2026

This was referenced Apr 9, 2026
WasmTestOnChrome: can't open file gen-debug-dump-docs.py #126706
Closed
wasm lanes fail with log error: Error message: Could not load file or assembly '/System.Private.CoreLib.dll'. The module was expected to contain an assembly manifest. (0x80131018) #126714
Closed

CopilotAI mentioned this pull request Apr 11, 2026
Fix WasmTestOnChrome: exclude browser/wasi from gen-debug-dump-docs.py helix post-command #126789
Merged
1 task

jakobbotsch approved these changes Apr 15, 2026
View reviewed changes

jakobbotsch merged commit dea4756 into dotnet:mainApr 15, 2026
144 of 148 checks passed

dotnet-maestroBot mentioned this pull request Apr 16, 2026
[main] Source code updates from dotnet/runtime dotnet/dotnet#6035
Merged

CopilotAI mentioned this pull request Apr 27, 2026
JIT: Restore EH-region check in if-conversion flow validation #127450
Merged

github-actionsBot locked and limited conversation to collaborators May 15, 2026