Skip to content

Commit

Permalink
JIT: fix case where RBO leads to an invalid CSE (dotnet#88159)
Browse files Browse the repository at this point in the history
If phi-based RBO bypasses a block with a memory PHI, it is possible for CSE to find
invalid memory-based CSEs. An example of this is seen in the attached test case.

Ideally perhaps CSE would kill availability of these CSEs at the point where memory
can change, but that seems difficult to arrange. Instead, we mark the bypased block
as one that will not propagate any incoming CSEs, as the failures we know of require
CSEs to flow back through this block.

Fixes dotnet#88091.
  • Loading branch information
AndyAyersMS committed Jun 29, 2023
1 parent 5a24757 commit bba7a9c
Show file tree
Hide file tree
Showing 7 changed files with 229 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/coreclr/jit/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ enum BasicBlockFlags : unsigned __int64
BBF_HAS_HISTOGRAM_PROFILE = MAKE_BBFLAG(39), // BB contains a call needing a histogram profile
BBF_TAILCALL_SUCCESSOR = MAKE_BBFLAG(40), // BB has pred that has potential tail call
BBF_RECURSIVE_TAILCALL = MAKE_BBFLAG(41), // Block has recursive tailcall that may turn into a loop
BBF_NO_CSE_IN = MAKE_BBFLAG(42), // Block should kill off any incoming CSE

// The following are sets of flags.

Expand Down
43 changes: 43 additions & 0 deletions src/coreclr/jit/fgdiagnostic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -941,6 +941,49 @@ bool Compiler::fgDumpFlowGraph(Phases phase, PhasePosition pos)
}
}

// Optionally show GC Heap Mem SSA state and Memory Phis
//
if ((JitConfig.JitDumpFgMemorySsa() != 0) && (fgSsaPassesCompleted > 0))
{
fprintf(fgxFile, "\\n");

MemoryKind k = MemoryKind::GcHeap;
const unsigned ssaIn = block->bbMemorySsaNumIn[k];
const unsigned ssaOut = block->bbMemorySsaNumOut[k];

if (ssaIn != SsaConfig::RESERVED_SSA_NUM)
{
ValueNum vnIn = GetMemoryPerSsaData(ssaIn)->m_vnPair.GetLiberal();
BasicBlock::MemoryPhiArg* memPhi = block->bbMemorySsaPhiFunc[k];
if ((memPhi != nullptr) && (memPhi != BasicBlock::EmptyMemoryPhiDef))
{
fprintf(fgxFile, "MI %d " FMT_VN " = PHI(", ssaIn, vnIn);
bool first = true;
for (; memPhi != nullptr; memPhi = memPhi->m_nextArg)
{
ValueNum phiVN = GetMemoryPerSsaData(memPhi->GetSsaNum())->m_vnPair.GetLiberal();
fprintf(fgxFile, "%s%d " FMT_VN, first ? "" : ",", memPhi->m_ssaNum, phiVN);
first = false;
}
fprintf(fgxFile, ")");
}
else
{
ValueNum vn = GetMemoryPerSsaData(block->bbMemorySsaNumIn[k])->m_vnPair.GetLiberal();
fprintf(fgxFile, "MI %d " FMT_VN, block->bbMemorySsaNumIn[k], vn);
}
fprintf(fgxFile, "\\n");

if (block->bbMemoryHavoc != 0)
{
fprintf(fgxFile, "** HAVOC **\\n");
}

ValueNum vnOut = GetMemoryPerSsaData(ssaOut)->m_vnPair.GetLiberal();
fprintf(fgxFile, "MO %d " FMT_VN, ssaOut, vnOut);
}
}

if (block->bbJumpKind == BBJ_COND)
{
fprintf(fgxFile, "\\n");
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/jitconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ CONFIG_INTEGER(JitDumpFgBlockFlags, W("JitDumpFgBlockFlags"), 0) // 0 == don't d
CONFIG_INTEGER(JitDumpFgLoopFlags, W("JitDumpFgLoopFlags"), 0) // 0 == don't display loop flags; 1 == display flags
CONFIG_INTEGER(JitDumpFgBlockOrder, W("JitDumpFgBlockOrder"), 0) // 0 == bbNext order; 1 == bbNum order; 2 == bbID
// order
CONFIG_INTEGER(JitDumpFgMemorySsa, W("JitDumpFgMemorySsa"), 0) // non-zero: show memory phis + SSA/VNs

CONFIG_STRING(JitLateDisasmTo, W("JITLateDisasmTo"))
CONFIG_STRING(JitRange, W("JitRange"))
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/optcse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,13 @@ class CSE_DataFlow
//
bool EndMerge(BasicBlock* block)
{
// If this block is marked BBF_NO_CSE_IN (because of RBO), kill all CSEs.
//
if ((block->bbFlags & BBF_NO_CSE_IN) != 0)
{
BitVecOps::ClearD(m_comp->cseLivenessTraits, block->bbCseIn);
}

// We can skip the calls kill step when our block doesn't have a callsite
// or we don't have any available CSEs in our bbCseIn
//
Expand Down
28 changes: 28 additions & 0 deletions src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,7 @@ struct JumpThreadInfo
, m_numTruePreds(0)
, m_numFalsePreds(0)
, m_ambiguousVN(ValueNumStore::NoVN)
, m_isPhiBased(false)
{
}

Expand Down Expand Up @@ -750,6 +751,8 @@ struct JumpThreadInfo
int m_numFalsePreds;
// Refined VN for ambiguous cases
ValueNum m_ambiguousVN;
// True if this was a phi-based jump thread
bool m_isPhiBased;
};

//------------------------------------------------------------------------
Expand Down Expand Up @@ -1270,6 +1273,8 @@ bool Compiler::optJumpThreadPhi(BasicBlock* block, GenTree* tree, ValueNum treeN
// see if the relop value is correlated with the pred.
//
JumpThreadInfo jti(this, block);
jti.m_isPhiBased = true;

for (BasicBlock* const predBlock : block->PredBlocks())
{
jti.m_numPreds++;
Expand Down Expand Up @@ -1588,6 +1593,29 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti)
}
}

// If this is a phi-based threading, and the block we're bypassing has
// a memory phi, mark the block with BBF_NO_CSE_IN so we can block CSE propagation
// into the block.
//
if (jti.m_isPhiBased)
{
for (MemoryKind memoryKind : allMemoryKinds())
{
if ((memoryKind == ByrefExposed) && byrefStatesMatchGcHeapStates)
{
continue;
}

if (jti.m_block->bbMemorySsaPhiFunc[memoryKind] != nullptr)
{
JITDUMP(FMT_BB " has %s memory phi; marking as BBF_NO_CSE_IN\n", jti.m_block->bbNum,
memoryKindNames[memoryKind]);
jti.m_block->bbFlags |= BBF_NO_CSE_IN;
break;
}
}
}

// If block didn't get fully optimized, and now has just one pred, see if
// we can sharpen the predicate's VN.
//
Expand Down
Loading

0 comments on commit bba7a9c

Please sign in to comment.