JIT: If-conversion phase can produce questionable IR. Return block having a successor

[BoyBaykiller]

I am doing some work on if-conversion and noticed that it can produce IR where a block with a return statement has a successor. Consider this minimal reproducible:

static int InvalidIR(bool cond)
{
    if (cond)
    {
        return 2;
    }
    // or else {}
    return 4;
}

Input to if-conversion:

------------ BB01 [0000] [000..003) -> BB03(0.5),BB02(0.5) (cond), preds={} succs={BB02,BB03}

***** BB01 [0000]
STMT00000 ( 0x000[E--] ... 0x001 )
N005 (  7,  8) [000003] -----+-----                         *  JTRUE     void   $VN.Void
N004 (  5,  6) [000002] J----+-N---                         \--*  EQ        int    $101
N002 (  3,  4) [000008] -----+-----                            +--*  CAST      int <- ubyte <- int $100
N001 (  2,  2) [000000] -----+-----                            |  \--*  LCL_VAR   int    V00 arg0         u:1 (last use) $80
N003 (  1,  1) [000001] -----+-----                            \--*  CNS_INT   int    0 $40

------------ BB02 [0001] [003..005) (return), preds={BB01} succs={}

***** BB02 [0001]
STMT00002 ( 0x003[E--] ... 0x004 )
N002 (  2,  2) [000007] -----+-----                         *  RETURN    int    $VN.Void
N001 (  1,  1) [000006] -----+-----                         \--*  CNS_INT   int    2 $44

------------ BB03 [0002] [005..007) (return), preds={BB01} succs={}

***** BB03 [0002]
STMT00001 ( 0x005[E--] ... 0x006 )
N002 (  2,  2) [000005] -----+-----                         *  RETURN    int    $VN.Void
N001 (  1,  1) [000004] -----+-----                         \--*  CNS_INT   int    4 $43

Then-case is BB02, else-case is BB03. Their statements are set to null and the SELECT node gets created in the JTRUE block (BB01). However the JTRUE block only get's it BB02 successor removed (and is set to BBJ_ALWAYS):

runtime/src/coreclr/jit/ifconversion.cpp

Lines 767 to 770 in b613202

	FlowEdge* const removedEdge = m_compiler->fgRemoveAllRefPreds(m_startBlock->GetFalseTarget(), m_startBlock);
	FlowEdge* const retainedEdge = m_startBlock->GetTrueEdge();
	m_startBlock->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);
	m_compiler->fgRepairProfileCondToUncond(m_startBlock, retainedEdge, removedEdge);

Which leads to the following output where BB01 has a return statement and a successor at the same time:

------------ BB01 [0000] [000..003) -> BB03(1) (always), preds={} succs={BB03}

***** BB01 [0000]
STMT00000 ( 0x000[E--] ... 0x001 )
N001 (  0,  0) [000003] -----+-----                         *  NOP       void  

***** BB01 [0000]
STMT00002 ( 0x003[E--] ... 0x004 )
N008 (  9, 10) [000007] -----+-----                         *  RETURN    int    $VN.Void
N007 (  8,  9) [000009] -----------                         \--*  SELECT    int   
N004 (  5,  6) [000002] J----+-N---                            +--*  EQ        int    $101
N002 (  3,  4) [000008] -----+-----                            |  +--*  CAST      int <- ubyte <- int $100
N001 (  2,  2) [000000] -----+-----                            |  |  \--*  LCL_VAR   int    V00 arg0         u:1 (last use) $80
N003 (  1,  1) [000001] -----+-----                            |  \--*  CNS_INT   int    0 $40
N005 (  1,  1) [000004] -----+-----                            +--*  CNS_INT   int    4 $43
N006 (  1,  1) [000006] -----+-----                            \--*  CNS_INT   int    2 $44

***** BB01 [0000]
STMT00001 ( 0x005[E--] ... 0x006 )
N001 (  0,  0) [000005] -----+-----                         *  NOP       void  

------------ BB02 [0001] [003..005) (return), preds={} succs={}

------------ BB03 [0002] [005..007) (return), preds={BB01} succs={}

---

I think this is also the reason it fails to recognize the second (from bottom to top) if-else flow here: https://godbolt.org/z/Kb5854sM1. So fixing this should make it catch a few more cases. But I am not sure how to do so in practice.

[dotnet-policy-service]

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

[jakobbotsch]

cc @a74nh. Seems like if-conversion is producing illegal IR (GT_RETURN that is not a terminator node and is not in a BBJ_RETURN block).

[a74nh]

I'll take a look.
This doesn't sound urgent, but it'll be good to fix it

[jakobbotsch]

I would consider this quite high priority. It is hard to reason about JIT behavior when invalid IR is being produced. It can result in all kinds of behavior.

[BoyBaykiller]

@a74nh Just letting you know that I am happy to work on this as part of some other changes. Feel free to still investigate yourself of course. This is what I have right now which seems to do the job. No more invalid IR and correct weights (in InvalidIR() bbWeight was 0.5):

// Update the flow graph. JTRUE block now contains the SELECT.
//

auto RemoveFlowInto = [&](BasicBlock* block)
{
    m_compiler->fgRemoveAllRefPreds(block, m_jtrueBlock);
    block->setBBProfileWeight(0.0);
    assert(block->bbPreds == nullptr);
};

// Remove flow into then/else blocks and update their weights
RemoveFlowInto(m_jtrueBlock->GetFalseTarget());
if (m_doElseConversion)
{
    RemoveFlowInto(m_jtrueBlock->GetTrueTarget());
}

// Change kind of JTRUE block and make it flow 
// directly into flow-converge block (which is null in case of GT_RETURN)
if (m_mainOper == GT_RETURN)
{
    m_jtrueBlock->SetKindAndTargetEdge(BBJ_RETURN);
}
else
{
    FlowEdge* newEdge = m_doElseConversion ? m_compiler->fgAddRefPred(m_flowsMergeBlock, m_jtrueBlock) : m_jtrueBlock->GetTrueEdge();
    m_jtrueBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);
}

assert(m_jtrueBlock->GetUniqueSucc() == m_flowsMergeBlock);

[a74nh]

@a74nh Just letting you know that I am happy to work on this as part of some other changes. Feel free to still investigate yourself of course. This is what I have right now which seems to do the job. No more invalid IR and correct weights (in InvalidIR() bbWeight was 0.5):

I was looking at this on Friday. I have similar code eg checking for m_doElseConversion and GT_RETURN, but mine is crashing for me in more complicated scenarios. If your version works, I'm happy for you to create a PR for it.