Skip to content

Commit 7f3f5ef

Browse files
AndyAyersMSAndyAyersMS
authored
JIT: fix remaining phases that were rebuilding pred edges (#80769)
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 #49030. Also fixes #80772.
1 parent 38ac350 commit 7f3f5ef

File tree

5 files changed

+70
-82
lines changed

5 files changed

+70
-82
lines changed

‎src/coreclr/jit/compiler.h‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1512,6 +1512,24 @@ enum API_ICorJitInfo_Names
15121512
API_COUNT
15131513
};
15141514

1515+
enum class FlowGraphUpdates
1516+
{
1517+
COMPUTE_BASICS = 0, // renumber blocks, reachability, etc
1518+
COMPUTE_DOMS = 1 << 0, // recompute dominators
1519+
COMPUTE_RETURNS = 1 << 1, // recompute return blocks
1520+
COMPUTE_LOOPS = 1 << 2, // recompute loop table
1521+
};
1522+
1523+
inline constexpr FlowGraphUpdates operator|(FlowGraphUpdates a, FlowGraphUpdates b)
1524+
{
1525+
return (FlowGraphUpdates)((unsigned int)a | (unsigned int)b);
1526+
}
1527+
1528+
inline constexpr FlowGraphUpdates operator&(FlowGraphUpdates a, FlowGraphUpdates b)
1529+
{
1530+
return (FlowGraphUpdates)((unsigned int)a & (unsigned int)b);
1531+
}
1532+
15151533
//---------------------------------------------------------------
15161534
// Compilation time.
15171535
//
@@ -5142,12 +5160,10 @@ class Compiler
51425160
// && postOrder(A) >= postOrder(B) making the computation O(1).
51435161
void fgNumberDomTree(DomTreeNode* domTree);
51445162

5145-
// When the flow graph changes, we need to update the block numbers, predecessor lists, reachability sets,
5163+
// When the flow graph changes, we need to update the block numbers, reachability sets,
51465164
// dominators, and possibly loops.
5147-
void fgUpdateChangedFlowGraph(const bool computePreds = true,
5148-
const bool computeDoms = true,
5149-
const bool computeReturnBlocks = false,
5150-
const bool computeLoops = false);
5165+
//
5166+
void fgUpdateChangedFlowGraph(FlowGraphUpdates updates);
51515167

51525168
public:
51535169
// Compute the predecessors of the blocks in the control flow graph.

‎src/coreclr/jit/fgopt.cpp‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -174,16 +174,19 @@ bool Compiler::fgReachable(BasicBlock* b1, BasicBlock* b2)
174174
// to be recomputed if they know certain things do NOT need to be recomputed.
175175
//
176176
// Arguments:
177-
// computePreds -- `true` if we should recompute predecessors
178-
// computeDoms -- `true` if we should recompute dominators
179-
// computeReturnBlocks -- `true` if we should recompute the list of return blocks
180-
// computeLoops -- `true` if we should recompute the loop table
181-
//
182-
void Compiler::fgUpdateChangedFlowGraph(const bool computePreds,
183-
const bool computeDoms,
184-
const bool computeReturnBlocks,
185-
const bool computeLoops)
177+
// updates -- enum flag set indicating what to update
178+
//
179+
// Notes:
180+
// Always renumbers, computes enter blocks, and computes reachability.
181+
// Optionally rebuilds dominators, return blocks, and computes loop information.
182+
//
183+
void Compiler::fgUpdateChangedFlowGraph(FlowGraphUpdates updates)
186184
{
185+
const bool computeDoms = ((updates & FlowGraphUpdates::COMPUTE_DOMS) == FlowGraphUpdates::COMPUTE_DOMS);
186+
const bool computeReturnBlocks =
187+
((updates & FlowGraphUpdates::COMPUTE_RETURNS) == FlowGraphUpdates::COMPUTE_RETURNS);
188+
const bool computeLoops = ((updates & FlowGraphUpdates::COMPUTE_LOOPS) == FlowGraphUpdates::COMPUTE_LOOPS);
189+
187190
// We need to clear this so we don't hit an assert calling fgRenumberBlocks().
188191
fgDomsComputed = false;
189192

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

195198
JITDUMP("\nRenumbering the basic blocks for fgUpdateChangeFlowGraph\n");
196199
fgRenumberBlocks();
197-
198-
if (computePreds) // This condition is only here until all phases don't require it.
199-
{
200-
fgComputePreds();
201-
}
202200
fgComputeEnterBlocksSet();
203201
fgComputeReachabilitySets();
204202
if (computeDoms)

‎src/coreclr/jit/flowgraph.cpp‎

Lines changed: 12 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -78,19 +78,9 @@ PhaseStatus Compiler::fgInsertGCPolls()
7878

7979
bool createdPollBlocks = false;
8080

81-
#ifdef DEBUG
82-
if (verbose)
83-
{
84-
printf("*************** In fgInsertGCPolls() for %s\n", info.compFullName);
85-
fgDispBasicBlocks(false);
86-
printf("\n");
87-
}
88-
#endif // DEBUG
89-
90-
BasicBlock* block;
91-
9281
// Walk through the blocks and hunt for a block that needs a GC Poll
93-
for (block = fgFirstBB; block != nullptr; block = block->bbNext)
82+
//
83+
for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
9484
{
9585
compCurBB = block;
9686

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

113103
GCPollType pollType = GCPOLL_INLINE;
114104

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

119109
if (opts.OptimizationDisabled())
120110
{
121-
#ifdef DEBUG
122-
if (verbose)
123-
{
124-
printf("Selecting CALL poll in block " FMT_BB " because of debug/minopts\n", block->bbNum);
125-
}
126-
#endif // DEBUG
127-
128111
// Don't split blocks and create inlined polls unless we're optimizing.
112+
//
113+
JITDUMP("Selecting CALL poll in block " FMT_BB " because of debug/minopts\n", block->bbNum);
129114
pollType = GCPOLL_CALL;
130115
}
131116
else if (genReturnBB == block)
132117
{
133-
#ifdef DEBUG
134-
if (verbose)
135-
{
136-
printf("Selecting CALL poll in block " FMT_BB " because it is the single return block\n", block->bbNum);
137-
}
138-
#endif // DEBUG
139-
140118
// we don't want to split the single return block
119+
//
120+
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is the single return block\n", block->bbNum);
141121
pollType = GCPOLL_CALL;
142122
}
143123
else if (BBJ_SWITCH == block->bbJumpKind)
144124
{
145-
#ifdef DEBUG
146-
if (verbose)
147-
{
148-
printf("Selecting CALL poll in block " FMT_BB " because it is a SWITCH block\n", block->bbNum);
149-
}
150-
#endif // DEBUG
151-
152125
// We don't want to deal with all the outgoing edges of a switch block.
126+
//
127+
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is a SWITCH block\n", block->bbNum);
153128
pollType = GCPOLL_CALL;
154129
}
155130
else if ((block->bbFlags & BBF_COLD) != 0)
156131
{
157-
#ifdef DEBUG
158-
if (verbose)
159-
{
160-
printf("Selecting CALL poll in block " FMT_BB " because it is a cold block\n", block->bbNum);
161-
}
162-
#endif // DEBUG
163-
164132
// We don't want to split a cold block.
133+
//
134+
JITDUMP("Selecting CALL poll in block " FMT_BB " because it is a cold block\n", block->bbNum);
165135
pollType = GCPOLL_CALL;
166136
}
167137

@@ -176,9 +146,7 @@ PhaseStatus Compiler::fgInsertGCPolls()
176146
{
177147
noway_assert(opts.OptimizationEnabled());
178148
fgReorderBlocks(/* useProfileData */ false);
179-
constexpr bool computePreds = true;
180-
constexpr bool computeDoms = false;
181-
fgUpdateChangedFlowGraph(computePreds, computeDoms);
149+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_BASICS);
182150
}
183151

184152
return result;
@@ -194,7 +162,6 @@ PhaseStatus Compiler::fgInsertGCPolls()
194162
// Return Value:
195163
// If new basic blocks are inserted, the last inserted block; otherwise, the input block.
196164
//
197-
198165
BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
199166
{
200167
bool createdPollBlocks;

‎src/coreclr/jit/loopcloning.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3286,9 +3286,8 @@ PhaseStatus Compiler::optCloneLoops()
32863286
if (optLoopsCloned > 0)
32873287
{
32883288
JITDUMP("Recompute reachability and dominators after loop cloning\n");
3289-
constexpr bool computePreds = false;
32903289
// TODO: recompute the loop table, to include the slow loop path in the table?
3291-
fgUpdateChangedFlowGraph(computePreds);
3290+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
32923291
}
32933292

32943293
#ifdef DEBUG

‎src/coreclr/jit/optimizer.cpp‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2684,8 +2684,7 @@ void Compiler::optFindNaturalLoops()
26842684
}
26852685
if (mod)
26862686
{
2687-
constexpr bool computePreds = true;
2688-
fgUpdateChangedFlowGraph(computePreds);
2687+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
26892688
}
26902689

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

26992698
if (fgModified)
27002699
{
2701-
// The predecessors were maintained in fgCreateLoopPreHeader; don't rebuild them.
2702-
constexpr bool computePreds = false;
2703-
constexpr bool computeDoms = true;
2704-
fgUpdateChangedFlowGraph(computePreds, computeDoms);
2700+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
27052701
}
27062702
}
27072703

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

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

3081+
fgRemoveRefPred(t, h);
3082+
fgAddRefPred(t, newH);
3083+
fgAddRefPred(newH, h);
3084+
30853085
// Anything that flows into sibling will flow here.
30863086
// So we use sibling.H as our best guess for weight.
30873087
//
@@ -3259,6 +3259,10 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
32593259
const bool extendRegion = BasicBlock::sameTryRegion(t, b);
32603260
BasicBlock* const newT = fgNewBBbefore(BBJ_NONE, t, extendRegion);
32613261

3262+
fgRemoveRefPred(t, h);
3263+
fgAddRefPred(t, newT);
3264+
fgAddRefPred(newT, h);
3265+
32623266
// Initially give newT the same weight as t; we will subtract from
32633267
// this for each edge that does not move from t to newT.
32643268
//
@@ -3308,7 +3312,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
33083312
JITDUMP("in optCanonicalizeLoop (current): redirect bottom->top backedge " FMT_BB " -> " FMT_BB
33093313
" to " FMT_BB " -> " FMT_BB "\n",
33103314
topPredBlock->bbNum, t->bbNum, topPredBlock->bbNum, newT->bbNum);
3311-
optRedirectBlock(b, blockMap);
3315+
optRedirectBlock(b, blockMap, RedirectBlockOption::UpdatePredLists);
33123316
}
33133317
}
33143318
else if (option == LoopCanonicalizationOption::Outer)
@@ -3340,7 +3344,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati
33403344
" -> " FMT_BB "\n",
33413345
topPredBlock == h ? "head" : "nonloop", topPredBlock == h ? "" : "back", topPredBlock->bbNum,
33423346
t->bbNum, topPredBlock->bbNum, newT->bbNum);
3343-
optRedirectBlock(topPredBlock, blockMap);
3347+
optRedirectBlock(topPredBlock, blockMap, RedirectBlockOption::UpdatePredLists);
33443348
}
33453349
}
33463350
else
@@ -4464,7 +4468,7 @@ PhaseStatus Compiler::optUnrollLoops()
44644468
optRedirectBlock(newBlock, &blockMap, RedirectBlockOption::AddToPredLists);
44654469
}
44664470

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

45124516
// Scrub all pred list references to block, except for bottom-> bottom->bbNext.
45134517
//
4514-
for (BasicBlock* succ : block->Succs())
4518+
for (BasicBlock* succ : block->Succs(this))
45154519
{
45164520
if ((block == bottom) && (succ == bottom->bbNext))
45174521
{
@@ -4639,12 +4643,16 @@ PhaseStatus Compiler::optUnrollLoops()
46394643

46404644
// If we unrolled any nested loops, we rebuild the loop table (including recomputing the
46414645
// return blocks list).
4642-
4643-
constexpr bool computePreds = false;
4644-
constexpr bool computeDoms = true;
4645-
const bool computeReturnBlocks = anyNestedLoopsUnrolled;
4646-
const bool computeLoops = anyNestedLoopsUnrolled;
4647-
fgUpdateChangedFlowGraph(computePreds, computeDoms, computeReturnBlocks, computeLoops);
4646+
//
4647+
if (anyNestedLoopsUnrolled)
4648+
{
4649+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS | FlowGraphUpdates::COMPUTE_RETURNS |
4650+
FlowGraphUpdates::COMPUTE_LOOPS);
4651+
}
4652+
else
4653+
{
4654+
fgUpdateChangedFlowGraph(FlowGraphUpdates::COMPUTE_DOMS);
4655+
}
46484656

46494657
DBEXEC(verbose, fgDispBasicBlocks());
46504658
}

0 commit comments

Comments
 (0)