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.

Copilot · 2026-03-11T18:25:04Z

src/coreclr/jit/ifconversion.cpp

+    m_doElseConversion = trueBb->GetUniquePred(m_compiler) != nullptr;
+    m_finalBlock       = m_doElseConversion ? trueBb->GetUniqueSucc() : trueBb;
+    m_mainOper         = GT_STORE_LCL_VAR;

-    for (int thenLimit = 0; thenLimit < m_checkLimit; thenLimit++)
+    if (m_finalBlock == nullptr)
    {
-        if (!IfConvertCheckInnerBlockFlow(thenBlock))
+        assert(m_doElseConversion);
+        if (falseBb->KindIs(BBJ_RETURN) && trueBb->KindIs(BBJ_RETURN))
        {
-            // Then block is not in a valid flow.
-            return true;
+            m_mainOper = GT_RETURN;
        }
-        BasicBlock* thenBlockNext = thenBlock->GetUniqueSucc();
-
-        if (thenBlockNext == m_finalBlock)
-        {
-            // All the Then blocks up to m_finalBlock are in a valid flow.
-            m_flowFound = true;
-            if (thenBlock->KindIs(BBJ_RETURN))
-            {
-                assert(m_finalBlock == nullptr);
-                m_mainOper = GT_RETURN;
-            }
-            else
-            {
-                m_mainOper = GT_STORE_LCL_VAR;
-            }
-            return true;
-        }
-
-        if (thenBlockNext == nullptr)
+        else
        {
-            // Invalid Then and Else combination.
            return false;
        }
-
-        thenBlock = thenBlockNext;
    }

-    // Nothing found. Still valid to continue.
-    return true;
-}
-
-//-----------------------------------------------------------------------------
-// IfConvertFindFlow
-//
-// Find a valid if conversion flow from m_startBlock to a final block.
-// There might be multiple Then and Else blocks in the flow - use m_checkLimit to limit this.
-//
-// Notes:
-//   Sets m_flowFound, m_finalBlock, m_doElseConversion and m_mainOper.
-//
-void OptIfConversionDsc::IfConvertFindFlow()
-{
-    // First check for flow with no else case. The final block is the destination of the jump.
-    m_doElseConversion = false;
-    m_finalBlock       = m_startBlock->GetTrueTarget();
-    assert(m_finalBlock != nullptr);
-    if (!IfConvertCheckThenFlow() || m_flowFound)
-    {
-        // Either the flow is invalid, or a flow was found.
-        return;
-    }
-
-    // Look for flows with else blocks. The final block is the block after the else block.
-    m_doElseConversion = true;
-    for (int elseLimit = 0; elseLimit < m_checkLimit; elseLimit++)
-    {
-        BasicBlock* elseBlock = m_finalBlock;
-        if (elseBlock == nullptr || !IfConvertCheckInnerBlockFlow(elseBlock))
-        {
-            // Need a valid else block in a valid flow .
-            return;
-        }
-
-        m_finalBlock = elseBlock->GetUniqueSucc();
-
-        if (!IfConvertCheckThenFlow() || m_flowFound)
-        {
-            // Either the flow is invalid, or a flow was found.
-            return;
-        }
-    }
+    return falseBb->GetUniqueSucc() == m_finalBlock;


IfConvertCheckFlow now requires the Then/Else blocks to be direct predecessors of the merge (via GetUniqueSucc equality). However the JIT can introduce empty BBJ_ALWAYS fallthrough blocks (e.g., BBF_INTERNAL blocks from fgNormalizeEH), which previously would have been handled by walking a short chain. This makes the change unlikely to be truly “zero-diff” and can silently disable if-conversion for methods with EH normalization artifacts. Consider skipping/peeling empty fallthrough blocks (similar to SkipFallthroughBlocks in switchrecognition.cpp) or reintroducing a bounded walk so the optimization remains robust in the presence of empty 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 .

int a74nh(bool x, bool y)
{
    if (x)
    {
        if (y)
        {
            return 1;
        }
        else
        {
            return 2;
        }
    }
    else
    {
        return 3;
    }
}

       mov      eax, 3
       mov      ecx, 2
       mov      r10d, 1
       test     r8b, r8b
       cmovne   ecx, r10d
       test     dl, dl
       cmovne   eax, ecx
						;; size=28 bbWeight=1 PerfScore 1.75
G_M25408_IG03:  ;; offset=0x001C
       ret

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

a74nh · 2026-03-16T09:42:37Z

src/coreclr/jit/ifconversion.cpp

The empty blocks in this comment bloc (here and below) need removing

Updated IR in the comment bloc

a74nh · 2026-03-16T09:54:55Z

src/coreclr/jit/ifconversion.cpp

    GenTree* last = m_startBlock->lastStmt()->GetRootNode();
-    noway_assert(last->OperIs(GT_JTRUE));
-    m_cond = last->gtGetOp1();
-    if (!m_cond->OperIsCompare())


There are no longer any checks for the contents Op1 of JTRUE. You are making the assumption that it must be a comparison. Could you add assert(last->gtGetOp1()->OperIsCompare); just to be sure.

Added assert(m_cond->OperIsCompare());

a74nh · 2026-03-16T09:57:00Z

src/coreclr/jit/ifconversion.cpp

-    BasicBlock* thenBlock = m_startBlock->GetFalseTarget();
+    m_doElseConversion = trueBb->GetUniquePred(m_compiler) != nullptr;
+    m_finalBlock       = m_doElseConversion ? trueBb->GetUniqueSucc() : trueBb;
+    m_mainOper         = GT_STORE_LCL_VAR;


Could you add an assert somewhere in this function to confirm the stmt is a GT_STORE_LCL_VAR

ah - that is checked on line 384. Not sure if it'd be better placed in here.

Yeah we can only assert that once we Checked the stmts. Or do you want to get m_mainOper from CheckStmts itself instead of CheckFlow. I was thinking of moving CheckStmts and CheckFlow into one function. Not sure what to name it though? It would return true if a if-conversion can be applied and assign all fields.

I did that:

IfConvertCheckFlow + IfConvertCheckStmts are bundled in a higher level function: IfConvertCheck (I am very open to renaming(s)). This will be good for JIT: Treat store in JTRUE block as the ElseOperation in if-conversion #124738

Instead of getting m_mainOper from the flow check it now directly comes from the statements check. So we have:

m_mainOper = m_thenOperation.node->OperGet(); assert(m_mainOper == GT_RETURN || m_mainOper == GT_STORE_LCL_VAR);

IfConvertCheck (I am very open to renaming(s))

it is ok - it does exactly what it says.

This will be good for JIT: Treat store in JTRUE block as the ElseOperation in if-conversion #124738

If you're continuing down this road then maybe you want to consider more than one statement inside the if.
GCC will if convert https://godbolt.org/z/18YqGbE43, but not if you uncomment the a3 statement.

(not for this PR)

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

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

github-actions bot 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-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 9, 2026

build-analysis bot mentioned this pull request Mar 9, 2026

Build error: ilc exited with code 57005 #124976

Open

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 5c1616c Compare 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

Open

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

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

Copilot AI reviewed Mar 11, 2026

View reviewed changes

build-analysis bot mentioned this pull request Mar 12, 2026

MsQuic fails with QUIC_STATUS_OUT_OF_MEMORY on AzureLinux #123216

Open

* plural to singluar in comment

43ed699

a74nh reviewed Mar 16, 2026

View reviewed changes

* 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

Open

Conversation

BoyBaykiller commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

BoyBaykiller commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a74nh commented Mar 13, 2026

Uh oh!

BoyBaykiller commented Mar 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BoyBaykiller commented Mar 9, 2026 •

edited

Loading

BoyBaykiller commented Mar 12, 2026 •

edited

Loading