Skip to content

Commit

Permalink
JIT: fix remaining phases that were rebuilding pred edges (dotnet#80769)
Browse files Browse the repository at this point in the history 
Loop canonicalization now maintains pred edges. GC poll insertion was already
maintaining the edges but was rebuilding them anyways.

Now pred lists are never rebuilt.

Also revise `fgUpdateChangedFlowGraph` so that it no longer has the
ability to remove or rebuild.

Fixes dotnet#49030.
Also fixes dotnet#80772.
  • Loading branch information
@AndyAyersMS @mdh1418
AndyAyersMS authored and mdh1418 committed Jan 24, 2023
1 parent 781f524 commit 416ca81
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 82 deletions.
26 changes: 21 additions & 5 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,24 @@ enum API_ICorJitInfo_Names
API_COUNT
};

enum class FlowGraphUpdates
{
COMPUTE_BASICS = 0, // renumber blocks, reachability, etc
COMPUTE_DOMS = 1 << 0, // recompute dominators
COMPUTE_RETURNS = 1 << 1, // recompute return blocks
COMPUTE_LOOPS = 1 << 2, // recompute loop table
};

inline constexpr FlowGraphUpdates operator|(FlowGraphUpdates a, FlowGraphUpdates b)
{
return (FlowGraphUpdates)((unsigned int)a | (unsigned int)b);
}

inline constexpr FlowGraphUpdates operator&(FlowGraphUpdates a, FlowGraphUpdates b)
{
return (FlowGraphUpdates)((unsigned int)a & (unsigned int)b);
}

//---------------------------------------------------------------
// Compilation time.
//
Expand Down Expand Up @@ -5142,12 +5160,10 @@ class Compiler
// && postOrder(A) >= postOrder(B) making the computation O(1).
void fgNumberDomTree(DomTreeNode* domTree);

// When the flow graph changes, we need to update the block numbers, predecessor lists, reachability sets,
// When the flow graph changes, we need to update the block numbers, reachability sets,
// dominators, and possibly loops.
void fgUpdateChangedFlowGraph(const bool computePreds = true,
const bool computeDoms = true,
const bool computeReturnBlocks = false,
const bool computeLoops = false);
//
void fgUpdateChangedFlowGraph(FlowGraphUpdates updates);

public:
// Compute the predecessors of the blocks in the control flow graph.
Expand Down
26 changes: 12 additions & 14 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,19 @@ bool Compiler::fgReachable(BasicBlock* b1, BasicBlock* b2)
// to be recomputed if they know certain things do NOT need to be recomputed.
//
// Arguments:
// computePreds -- `true` if we should recompute predecessors
// computeDoms -- `true` if we should recompute dominators
// computeReturnBlocks -- `true` if we should recompute the list of return blocks
// computeLoops -- `true` if we should recompute the loop table
//
void Compiler::fgUpdateChangedFlowGraph(const bool computePreds,
const bool computeDoms,
const bool computeReturnBlocks,
const bool computeLoops)
// updates -- enum flag set indicating what to update
//
// Notes:
// Always renumbers, computes enter blocks, and computes reachability.
// Optionally rebuilds dominators, return blocks, and computes loop information.
//
void Compiler::fgUpdateChangedFlowGraph(FlowGraphUpdates updates)
{
const bool computeDoms = ((updates & FlowGraphUpdates::COMPUTE_DOMS) == FlowGraphUpdates::COMPUTE_DOMS);
const bool computeReturnBlocks =
((updates & FlowGraphUpdates::COMPUTE_RETURNS) == FlowGraphUpdates::COMPUTE_RETURNS);
const bool computeLoops = ((updates & FlowGraphUpdates::COMPUTE_LOOPS) == FlowGraphUpdates::COMPUTE_LOOPS);

// We need to clear this so we don't hit an assert calling fgRenumberBlocks().
fgDomsComputed = false;

Expand All @@ -194,11 +197,6 @@ void Compiler::fgUpdateChangedFlowGraph(const bool computePreds,

JITDUMP("\nRenumbering the basic blocks for fgUpdateChangeFlowGraph\n");
fgRenumberBlocks();

if (computePreds) // This condition is only here until all phases don't require it.
{
fgComputePreds();
}
fgComputeEnterBlocksSet();
fgComputeReachabilitySets();
if (computeDoms)
Expand Down
57 changes: 12 additions & 45 deletions src/coreclr/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,19 +78,9 @@ PhaseStatus Compiler::fgInsertGCPolls()

bool createdPollBlocks = false;

#ifdef DEBUG
if (verbose)
{
printf("*************** In fgInsertGCPolls() for %s\n", info.compFullName);
fgDispBasicBlocks(false);
printf("\n");
}
#endif // DEBUG

BasicBlock* block;

// Walk through the blocks and hunt for a block that needs a GC Poll
for (block = fgFirstBB; block != nullptr; block = block->bbNext)
//
for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
{
compCurBB = block;

Expand All @@ -112,56 +102,36 @@ PhaseStatus Compiler::fgInsertGCPolls()

GCPollType pollType = GCPOLL_INLINE;

// We'd like to inset an inline poll. Below is the list of places where we
// We'd like to insert an inline poll. Below is the list of places where we
// can't or don't want to emit an inline poll. Check all of those. If after all of that we still
// have INLINE, then emit an inline check.

if (opts.OptimizationDisabled())
{
#ifdef DEBUG
if (verbose)
{
printf("Selecting CALL poll in block " FMT_BB " because of debug/minopts\n", block->bbNum);
}
#endif // DEBUG

// Don't split blocks and create inlined polls unless we're optimizing.
//
JITDUMP("Selecting CALL poll in block " FMT_BB " because of debug/minopts\n", block->bbNum);
pollType = GCPOLL_CALL;
}
else if (genReturnBB == block)
{
#ifdef DEBUG
if (verbose)
{
printf("Selecting CALL poll in block " FMT_BB " because it is the single return block\n", block->bbNum);
}
#endif // DEBUG

// we don't want to split the single return block
//
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is the single return block\n", block->bbNum);
pollType = GCPOLL_CALL;
}
else if (BBJ_SWITCH == block->bbJumpKind)
{
#ifdef DEBUG
if (verbose)
{
printf("Selecting CALL poll in block " FMT_BB " because it is a SWITCH block\n", block->bbNum);
}
#endif // DEBUG

// We don't want to deal with all the outgoing edges of a switch block.
//
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is a SWITCH block\n", block->bbNum);
pollType = GCPOLL_CALL;
}
else if ((block->bbFlags & BBF_COLD) != 0)
{
#ifdef DEBUG
if (verbose)
{
printf("Selecting CALL poll in block " FMT_BB " because it is a cold block\n", block->bbNum);
}
#endif // DEBUG

// We don't want to split a cold block.
//
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is a cold block\n", block->bbNum);
pollType = GCPOLL_CALL;
}

Expand All @@ -176,9 +146,7 @@ PhaseStatus Compiler::fgInsertGCPolls()
{
noway_assert(opts.OptimizationEnabled());
fgReorderBlocks(/* useProfileData */ false);
constexpr bool computePreds = true;
constexpr bool computeDoms = false;
fgUpdateChangedFlowGraph(computePreds, computeDoms);
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_BASICS);
}

return result;
Expand All @@ -194,7 +162,6 @@ PhaseStatus Compiler::fgInsertGCPolls()
// Return Value:
// If new basic blocks are inserted, the last inserted block; otherwise, the input block.
//

BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
{
bool createdPollBlocks;
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/loopcloning.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3286,9 +3286,8 @@ PhaseStatus Compiler::optCloneLoops()
if (optLoopsCloned > 0)
{
JITDUMP("Recompute reachability and dominators after loop cloning\n");
constexpr bool computePreds = false;
// TODO: recompute the loop table, to include the slow loop path in the table?
fgUpdateChangedFlowGraph(computePreds);
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
}

#ifdef DEBUG
Expand Down
40 changes: 24 additions & 16 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2684,8 +2684,7 @@ void Compiler::optFindNaturalLoops()
}
if (mod)
{
constexpr bool computePreds = true;
fgUpdateChangedFlowGraph(computePreds);
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
}

if (false /* pre-header stress */)
Expand All @@ -2698,10 +2697,7 @@ void Compiler::optFindNaturalLoops()

if (fgModified)
{
// The predecessors were maintained in fgCreateLoopPreHeader; don't rebuild them.
constexpr bool computePreds = false;
constexpr bool computeDoms = true;
fgUpdateChangedFlowGraph(computePreds, computeDoms);
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
}
}

Expand Down Expand Up @@ -3082,6 +3078,10 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd)

BasicBlock* const newH = fgNewBBbefore(BBJ_NONE, t, /*extendRegion*/ true);

fgRemoveRefPred(t, h);
fgAddRefPred(t, newH);
fgAddRefPred(newH, h);

// Anything that flows into sibling will flow here.
// So we use sibling.H as our best guess for weight.
//
Expand Down Expand Up @@ -3259,6 +3259,10 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
const bool extendRegion = BasicBlock::sameTryRegion(t, b);
BasicBlock* const newT = fgNewBBbefore(BBJ_NONE, t, extendRegion);

fgRemoveRefPred(t, h);
fgAddRefPred(t, newT);
fgAddRefPred(newT, h);

// Initially give newT the same weight as t; we will subtract from
// this for each edge that does not move from t to newT.
//
Expand Down Expand Up @@ -3308,7 +3312,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
JITDUMP("in optCanonicalizeLoop (current): redirect bottom->top backedge " FMT_BB " -> " FMT_BB
" to " FMT_BB " -> " FMT_BB "\n",
topPredBlock->bbNum, t->bbNum, topPredBlock->bbNum, newT->bbNum);
optRedirectBlock(b, blockMap);
optRedirectBlock(b, blockMap, RedirectBlockOption::UpdatePredLists);
}
}
else if (option == LoopCanonicalizationOption::Outer)
Expand Down Expand Up @@ -3340,7 +3344,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
" -> " FMT_BB "\n",
topPredBlock == h ? "head" : "nonloop", topPredBlock == h ? "" : "back", topPredBlock->bbNum,
t->bbNum, topPredBlock->bbNum, newT->bbNum);
optRedirectBlock(topPredBlock, blockMap);
optRedirectBlock(topPredBlock, blockMap, RedirectBlockOption::UpdatePredLists);
}
}
else
Expand Down Expand Up @@ -4464,7 +4468,7 @@ PhaseStatus Compiler::optUnrollLoops()
optRedirectBlock(newBlock, &blockMap, RedirectBlockOption::AddToPredLists);
}

// We fall into to this unroll iteration from the bottom block (first iteration)
// We fall into this unroll iteration from the bottom block (first iteration)
// or from the previous unroll clone of the bottom block (subsequent iterations).
// After doing this, all the newly cloned blocks now have proper flow and pred lists.
//
Expand Down Expand Up @@ -4511,7 +4515,7 @@ PhaseStatus Compiler::optUnrollLoops()

// Scrub all pred list references to block, except for bottom-> bottom->bbNext.
//
for (BasicBlock* succ : block->Succs())
for (BasicBlock* succ : block->Succs(this))
{
if ((block == bottom) && (succ == bottom->bbNext))
{
Expand Down Expand Up @@ -4639,12 +4643,16 @@ PhaseStatus Compiler::optUnrollLoops()

// If we unrolled any nested loops, we rebuild the loop table (including recomputing the
// return blocks list).

constexpr bool computePreds = false;
constexpr bool computeDoms = true;
const bool computeReturnBlocks = anyNestedLoopsUnrolled;
const bool computeLoops = anyNestedLoopsUnrolled;
fgUpdateChangedFlowGraph(computePreds, computeDoms, computeReturnBlocks, computeLoops);
//
if (anyNestedLoopsUnrolled)
{
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS | FlowGraphUpdates::COMPUTE_RETURNS |
FlowGraphUpdates::COMPUTE_LOOPS);
}
else
{
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
}

DBEXEC(verbose, fgDispBasicBlocks());
}
Expand Down

0 comments on commit 416ca81

Please sign in to comment.