JIT: Treat store in JTRUE block as the ElseOperation in if-conversion by BoyBaykiller · Pull Request #124738 · dotnet/runtime

BoyBaykiller · 2026-02-23T04:54:28Z

If-conversion phase now produces the same IR for these two cases:

bool First(int tMinLeft, int tMinRight)
{
    bool leftCloser = false;
    if (tMinLeft < tMinRight)
    {
        leftCloser = true;
    }
    return leftCloser;
}

bool Second(int tMinLeft, int tMinRight)
{
    bool leftCloser;
    if (tMinLeft < tMinRight)
    {
        leftCloser = true;
    }
    else
    {
        leftCloser = false;
    }
    return leftCloser;
}

The last unconditional store (leftCloser = false) is substituted into the SELECT which enables further optimization. Specifically it fixes #124713.

dotnet-policy-service · 2026-02-23T04:55:28Z

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

BoyBaykiller · 2026-02-23T04:55:48Z

@dotnet-policy-service agree

…xecuted, so not inside Else block

Copilot

Pull request overview

This PR updates the CoreCLR JIT if-conversion phase to treat an unconditional prior store in the JTRUE block as the effective “else” value when forming a SELECT, so equivalent C# patterns yield equivalent IR and enable downstream optimizations (per #124713).

Changes:

Replaces explicit m_doElseConversion state with a HasElseBlock() helper and uses m_elseOperation.block != nullptr to drive behavior.
Adds logic to locate a prior invariant STORE_LCL_VAR in the JTRUE block and use it as an ElseOperation when there is no explicit else block.
Adjusts debug dumping/logging and CFG cleanup to handle both explicit else blocks and synthesized else operations.

Copilot · 2026-04-24T23:59:09Z

+    {
+        // There is no Else block, but we can still find an Else operation. Search for
+        // most recent STORE to the local in JTRUE block and see if it's legal to fwd sub
+        // it's definition into the SELECT and remove the STORE.


In this comment, "it's" should be the possessive "its" (no apostrophe).

Suggested change

// it's definition into the SELECT and remove the STORE.

// its definition into the SELECT and remove the STORE.

Copilot · 2026-04-24T23:59:10Z

@@ -535,28 +590,36 @@ bool OptIfConversionDsc::optIfConvert(int* pReachabilityBudget)

    // JTRUE block now contains SELECT. Change it's kind and make it flow


In this comment, "Change it's kind" should use the possessive "its" (no apostrophe).

Suggested change

// JTRUE block now contains SELECT. Change it's kind and make it flow

// JTRUE block now contains SELECT. Change its kind and make it flow

Copilot · 2026-04-24T23:59:10Z

+        // There is no Else block, but we can still find an Else operation. Search for
+        // most recent STORE to the local in JTRUE block and see if it's legal to fwd sub
+        // it's definition into the SELECT and remove the STORE.
+
+        assert(m_mainOper == GT_STORE_LCL_VAR);
+
+        GenTreeLclVar* thenStore    = m_thenOperation.node->AsLclVar();
+        unsigned       targetLclNum = thenStore->GetLclNum();
+
+        bool usedInThenBlock = m_compiler->gtHasRef(thenStore->Data(), targetLclNum);
+        if (!usedInThenBlock)
+        {
+            Statement* last = m_startBlock->lastStmt();
+            Statement* stmt = last;
+            do
+            {
+                GenTree* tree = stmt->GetRootNode();
+                if (tree->OperIs(GT_STORE_LCL_VAR))
+                {
+                    GenTreeLclVar* prevStore = tree->AsLclVar();
+                    if (prevStore->GetLclNum() == targetLclNum)
+                    {
+                        if (prevStore->Data()->IsInvariant())
+                        {
+                            m_elseOperation.block = m_startBlock;
+                            m_elseOperation.stmt  = stmt;
+                            m_elseOperation.node  = tree;
+                        }
+
+                        break;
+                    }
+                }
+
+                if (m_compiler->gtHasRef(tree, targetLclNum))
+                {
+                    break;
+                }
+
+                stmt = stmt->GetPrevStmt();
+            } while (stmt != last);
+        }


The new path that synthesizes an ElseOperation from a previous STORE in the JTRUE block (when there is no explicit else block) changes if-conversion behavior but isn't covered by a JIT disasm-check regression test. Consider adding a small test case for the "bool x=false; if(cond){x=true;}" pattern to ensure it if-converts into a SELECT and enables the expected setcc/codegen improvement, so this doesn’t regress silently.

jakobbotsch · 2026-04-27T10:12:22Z

+            Statement* last = m_startBlock->lastStmt();
+            Statement* stmt = last;
+            do
+            {
+                GenTree* tree = stmt->GetRootNode();
+                if (tree->OperIs(GT_STORE_LCL_VAR))
+                {
+                    GenTreeLclVar* prevStore = tree->AsLclVar();
+                    if (prevStore->GetLclNum() == targetLclNum)
+                    {
+                        if (prevStore->Data()->IsInvariant())
+                        {
+                            m_elseOperation.block = m_startBlock;
+                            m_elseOperation.stmt  = stmt;
+                            m_elseOperation.node  = tree;
+                        }
+
+                        break;
+                    }
+                }
+
+                if (m_compiler->gtHasRef(tree, targetLclNum))
+                {
+                    break;
+                }
+
+                stmt = stmt->GetPrevStmt();
+            } while (stmt != last);


An unbounded IR search like this is concerning. I would not be surprised if this can introduce JIT hangs in pathological cases.
This change to walk the IR seems like an additional change that is not needed for this PR. Can it be done in a separate PR and follow-up so that it can be evaluated in isolation?

This sits at the heart of this PR. How should I find the most recent store into targetLclNum in m_startBlock without an IR walk?

The pattern you are targeting looks like:

x = foo; if (cond) x = bar;

I would not expect to need to look backwards more than one statement from the JTRUE for this pattern.

I would be ok with just bounding this walk to some constant, like try to find the statement within the previous 8 statements.

Ok let me try.

Given https://github.com/dotnet/runtime/pull/124738/changes#r3148881220 I would suggest not bothering. Instead I would propose to just validate that the conditional has no side effects (filters out both (2) and (3)) and then use gtTreeHasLocalRead to handle (1). If you want the search I suggest a follow-up PR.

jakobbotsch · 2026-04-27T16:47:20Z

+                    }
+                }
+
+                if (m_compiler->gtHasRef(tree, targetLclNum))


gtHasRef is not a sufficient check here for a few reasons:

It does not check for promoted cases, i.e. if targetLclNum is a field and the parent local has a use, then the value will still depend on the value of targetLclNum. See fgCanMoveFirstStatementIntoPred for how to handle this.

The local can have uses in EH successors that control gets transferred to by any intervening exception throw.

If it is address exposed then not all uses are visible, and you really cannot reason about it in any meaningful fashion when intervening statements have global side effects.

See fgCanMoveFirstStatementIntoPred for how to handle this.

Actually we have gtTreeHasLocalRead for a version of gtHasRef that handles promotion.

* update the 'is this lcl used?' check

BoyBaykiller · 2026-04-27T22:25:37Z

The amount of diffs got halved.
Old arm64 -25,036
New arm64 -16,324

I am not sure that's more because we now bail if condition has side effects or that we only check the immediate predecessor. Like you said I can investigate and possibly improve this in a future PR. @jakobbotsch

jakobbotsch · 2026-04-27T23:28:27Z

+        bool unusedInThen = !m_compiler->gtTreeHasLocalRead(thenStore->Data(), targetLclNum);
+        bool unusedInCond =
+            ((m_cond->gtFlags & GTF_SIDE_EFFECT) == 0) && !m_compiler->gtTreeHasLocalRead(m_cond, targetLclNum);


If targetLclNum is address exposed then these checks are not sufficient. GTF_SIDE_EFFECT needs to be GTF_GLOB_EFFECT and unusedInThen needs to also check GTF_GLOB_REF.
Or you can just skip the opt if targetLclNum is address exposed. It is unlikely to be a common scenario in the diffs.

* add budget to stmt search * ignore address exposed lcls

BoyBaykiller · 2026-04-28T07:11:39Z

The amount of diffs got halved. I am not sure that's more because we now bail if condition has side effects or that we only check the immediate predecessor

I tested locally and the majority comes from the GTF_SIDE_EFFECT check, however in the real world I expect the stmtSearchBudget to have a larger impact. I now ignore address exposed variables as you said and I wonder if I still need to check GTF_PERSISTENT_SIDE_EFFECTS | GTF_EXCEPT ( GTF_SIDE_EFFECT)

Regarding GTF_PERSISTENT_SIDE_EFFECTS: I use gtTreeHasLocalStore instead.

Regarding GTF_EXCEPT: Removing this brings by far the biggest diffs. You said "The local can have uses in EH successors that control gets transferred to by any intervening exception throw". I am trying to come up with a reproducible for this but I am having trouble. Conceptually I think you mean something like this:

void M0(int var1, ref bool throwingCond)
{
    var1 = 1;
    try
    {
        if (throwingCond) // <- might throw
        {
            var1 = 0;
        }
    }
    catch (Exception)
    {
        // used in EH
        var1 = 5;
    }

    Console.WriteLine(var1);
}

But in practice the var1 = 1; is not part of the same block as the JTRUE. So I am hoping we don't need GTF_EXCEPT either

BoyBaykiller · 2026-04-28T07:15:27Z

+        return false;
+    }
+
+    int        stmtSearchBudget = 2;


Note 2 includes the jtrue stmt itself. So we actually only look back 1 stmt

jakobbotsch · 2026-04-28T09:40:38Z

Regarding GTF_EXCEPT: Removing this brings by far the biggest diffs. You said "The local can have uses in EH successors that control gets transferred to by any intervening exception throw". I am trying to come up with a reproducible for this but I am having trouble. Conceptually I think you mean something like this:
void M0(int var1, ref bool throwingCond)
{
    var1 = 1;
    try
    {
        if (throwingCond) // <- might throw
        {
            var1 = 0;
        }
    }
    catch (Exception)
    {
        // used in EH
        var1 = 5;
    }

    Console.WriteLine(var1);
}
But in practice the var1 = 1; is not part of the same block as the JTRUE. So I am hoping we don't need GTF_EXCEPT either

Close, I mean something like:

using System;
using System.Runtime.CompilerServices;

public class Program
{
    public static void Main()
    {
        Foo(null);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Foo(C c)
    {
        int x = 0;
        try
        {
            x = 1;
            if (c.Field == 2)
                x = 2;
        }
        catch
        {
            Console.WriteLine(x);
        }
    }

    class C
    {
        public int Field;
    } 
}

Main: Prints 1
This PR: Prints 0

If this case is important for diffs you can use BasicBlock::HasPotentialEHSuccs. If false, control cannot resume in the same function after an exception is thrown. To be even more precise you can use LclVarDsc::lvLiveInOutOfHndlr, but I doubt the latter would bring many (if any) improvements, so I would not bother with the complexity.

BoyBaykiller · 2026-04-28T14:27:17Z

Thank you for the repro! That really helps my understanding.

If this case is important for diffs you can use BasicBlock::HasPotentialEHSuccs. If false, control cannot resume in the same function after an exception is thrown. To be even more precise you can use LclVarDsc::lvLiveInOutOfHndlr, but I doubt the latter would bring many (if any) improvements, so I would not bother with the complexity.

I didn't know these existed, that's helpful. lvLiveInOutOfHndlr gives me exactly what I needed and is free. I think having the precise check actually makes the code more self-documenting in a way. And it fits right in with the existing IsAddressExposed check, so I'd prefer that over BasicBlock::HasPotentialEHSuccs.

I guess we could be even more precise by checking for GTF_EXCEPT in combination with lvLiveInOutOfHndlr : ) But that would certainly add useless complexity so I wont do that.

jakobbotsch · 2026-04-28T14:59:24Z

+    // Cannot easily reason about all uses if it is address exposed or
+    // used in EH where control could get transferred to at "any time"
+    LclVarDsc* lclVarDsc = m_compiler->lvaGetDesc(targetLclNum);
+    if (lclVarDsc->IsAddressExposed() || lclVarDsc->lvLiveInOutOfHndlr)


lvLiveInOutOfHndlr is accurate only for tracked locals. It cannot be used without a check for lvTracked and fallback to more conservative behavior in the untracked case.

Oh ok lol. I am learning as I am going. Then I'll use BasicBlock::HasPotentialEHSuccs.

I would suggest HasPotentialEHSuccs combined with the exception check, yeah.

…-unconditional-store-to-else-124713

BoyBaykiller · 2026-04-28T18:37:13Z

I've made the EH check more precise to avoid spurious diffs as you said. Now this still gets if-converted:

void Foo(bool cond, C c)
{
    int x = 2;
    try
    {
        _ = c.Field; // GTF_EXCEPT before STORE, don't bail
        x = 0;
        if (cond)
            x = 1;
    }
    catch
    {
        Console.WriteLine(x);
    }
}

BoyBaykiller · 2026-04-28T21:18:48Z

The diffs look better again. Upping the stmtSearchBudget a little should get us back to where we were before, but if I understood correctly you wish to have that be a separate PR

jakobbotsch · 2026-04-28T21:22:55Z

The diffs look better again. Upping the stmtSearchBudget a little should get us back to where we were before, but if I understood correctly you wish to have that be a separate PR

I'm ok with doing it here given that we have all the machinery anyway to deal with the JTRUE interference.

jakobbotsch · 2026-04-29T10:13:08Z

+        if (((tree->gtFlags & GTF_EXCEPT) != 0) && hasEhSuccs)
+        {
+            break;
+        }


I think this should check for GTF_ORDER_SIDEEFF as well.

Why is that check needed? It makes the diffs bad again.

github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 23, 2026

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Feb 23, 2026

BoyBaykiller mentioned this pull request Feb 24, 2026

JIT: Make cmov codegen the same for these equivalent IR sequences #124785

Open

This was referenced Feb 24, 2026

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

Unable to pull image from mcr.microsoft.com #117164

Open

[Android][CoreCLR] System.Security.Cryptography.Tests killed by lowmemorykiller #118603

Open

BoyBaykiller marked this pull request as ready for review February 24, 2026 19:40

BoyBaykiller marked this pull request as draft February 24, 2026 20:01

BoyBaykiller changed the title ~~JIT: Treat unconditional store as the else case in if-conversion~~ JIT: Treat store in JTRUE block as the ElseOperation in if-conversion Mar 10, 2026

BoyBaykiller mentioned this pull request Mar 17, 2026

JIT: Simplify if-conversion #125347

Merged

BoyBaykiller force-pushed the if-conv-unconditional-store-to-else-124713 branch from 208cc34 to cba3e37 Compare March 19, 2026 00:48

BoyBaykiller mentioned this pull request Mar 22, 2026

JIT: Optimize some bitwise ops in context of enum flags #125899

Open

Copilot AI mentioned this pull request Mar 22, 2026

Fix intermittent NRE in ETW listener tests by using GUID overload of EnableProvider #125908

Merged

BoyBaykiller closed this Apr 15, 2026

BoyBaykiller force-pushed the if-conv-unconditional-store-to-else-124713 branch from 8997012 to 2ae217c Compare April 15, 2026 18:29

* treat store in JTRUE block as ElseOperation

2a9eccc

build-analysis Bot mentioned this pull request Apr 16, 2026

System.Net.NameResolution.Tests DNS failures: Name or service not known #126641

Open

BoyBaykiller and others added 2 commits April 24, 2026 17:25

Merge branch 'main' into if-conv-unconditional-store-to-else-124713

e16b809

* have ElseCost be 0 when ElseOperation was already unconditionally e…

6f8a69e

…xecuted, so not inside Else block

build-analysis Bot mentioned this pull request Apr 24, 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

3 tasks

EgorBo requested a review from Copilot April 24, 2026 23:49

Copilot started reviewing on behalf of EgorBo April 24, 2026 23:50 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

* fix typo

6009c10

build-analysis Bot mentioned this pull request Apr 25, 2026

XHarness package install failure on iOS due to devicectl NSPOSIXErrorDomain error 49 #123796

Open

jakobbotsch reviewed Apr 27, 2026

View reviewed changes

* only check immediate predecessor stmt of JTRUE

083b153

* update the 'is this lcl used?' check

jakobbotsch reviewed Apr 27, 2026

View reviewed changes

* factor into function

23feda4

* add budget to stmt search * ignore address exposed lcls

BoyBaykiller commented Apr 28, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Apr 28, 2026

Android arm32 device not found (armeabi-v7a architecture unavailable) #125440

Open

* make check more precise with lvLiveInOutOfHndlr

34b5381

jakobbotsch reviewed Apr 28, 2026

View reviewed changes

BoyBaykiller added 3 commits April 28, 2026 19:25

Merge branch 'main' of https://github.com/dotnet/runtime into if-conv…

3c3344f

…-unconditional-store-to-else-124713

* switch to HasPotentialEHSuccs

a748543

* make 'lcl used in EH' check more precise to avoid spurious diffs

46369f1

* stmtSearchBudget = 8 to catch some more cases

59ec9a5

jakobbotsch reviewed Apr 29, 2026

View reviewed changes

	// it's definition into the SELECT and remove the STORE.
	// its definition into the SELECT and remove the STORE.

		@@ -535,28 +590,36 @@ bool OptIfConversionDsc::optIfConvert(int* pReachabilityBudget)

		// JTRUE block now contains SELECT. Change it's kind and make it flow

Conversation

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

Uh oh!

Uh oh!

dotnet-policy-service Bot commented Feb 23, 2026

Uh oh!

BoyBaykiller commented Feb 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

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!

jakobbotsch Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jakobbotsch Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoyBaykiller commented Apr 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Apr 28, 2026

Uh oh!

BoyBaykiller commented Apr 28, 2026

Uh oh!

jakobbotsch Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

BoyBaykiller commented Apr 28, 2026

Uh oh!

BoyBaykiller commented Apr 28, 2026

Uh oh!

jakobbotsch commented Apr 28, 2026

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 Feb 23, 2026 •

edited

Loading

jakobbotsch Apr 27, 2026 •

edited

Loading

jakobbotsch Apr 27, 2026 •

edited

Loading

BoyBaykiller commented Apr 27, 2026 •

edited

Loading

jakobbotsch Apr 28, 2026 •

edited

Loading