From 26b2c7c4a8a34ef98d591e77085808cff55a199a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 15 Jun 2022 18:36:06 -0700 Subject: [PATCH 1/4] JIT: break loop canonicalization into two stages For a given loop, we need to separate out the true backedge, any non-loop backedges, and any inner loop backedges so that they all target distinct blocks. Otherwise we may violate assumptions that the loop entry dominates all blocks in the loop and that all backedges that reach top come from within the loop. This seems simplest to do with two rounds of canonicalization, one that moves the non-loop edges, and another that moves the true backedge. Fixes #70802. --- src/coreclr/jit/compiler.h | 8 + src/coreclr/jit/optimizer.cpp | 329 +++++++++++++++++++++++++--------- 2 files changed, 251 insertions(+), 86 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a7ece7e80e9cd3..22f84ad2583b9e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6334,6 +6334,14 @@ class Compiler // unshared with any other loop. Returns "true" iff the flowgraph has been modified bool optCanonicalizeLoop(unsigned char loopInd); + enum class LoopCanonicalizationOption + { + Outer, + Current + }; + + bool optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizationOption option); + // Requires "l1" to be a valid loop table index, and not "BasicBlock::NOT_IN_LOOP". // Requires "l2" to be a valid loop table index, or else "BasicBlock::NOT_IN_LOOP". // Returns true iff "l2" is not NOT_IN_LOOP, and "l1" contains "l2". diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index de2459c862e9a3..7c359a269255ed 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1725,9 +1725,14 @@ class LoopSearch // Thus, we have to be very careful and after entry discovery check that it is indeed // the only place we enter the loop (especially for non-reducible flow graphs). + JITDUMP("FindLoop: checking head:" FMT_BB " top:" FMT_BB " bottom:" FMT_BB "\n", + head->bbNum, top->bbNum, bottom->bbNum); + if (top->bbNum > bottom->bbNum) // is this a backward edge? (from BOTTOM to TOP) { // Edge from BOTTOM to TOP is not a backward edge + JITDUMP(" " FMT_BB "->" FMT_BB " is not a backedge\n", + bottom->bbNum, top->bbNum); return false; } @@ -1735,16 +1740,20 @@ class LoopSearch { // Not a true back-edge; bottom is a block added to reconnect fall-through during // loop processing, so its block number does not reflect its position. + JITDUMP(" " FMT_BB "->" FMT_BB " is not a true backedge\n", + bottom->bbNum, top->bbNum); return false; } if (bottom->KindIs(BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET, BBJ_CALLFINALLY, BBJ_SWITCH)) { + JITDUMP(" bottom odd jump kind\n"); // BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET, and BBJ_CALLFINALLY can never form a loop. // BBJ_SWITCH that has a backward jump appears only for labeled break. return false; } + // The presence of a "back edge" is an indication that a loop might be present here. // // Definition: A loop is: @@ -1759,6 +1768,7 @@ class LoopSearch if (entry == nullptr) { // For now, we only recognize loops where HEAD has some successor ENTRY in the loop. + JITDUMP(" can't find entry\n"); return false; } @@ -1773,12 +1783,14 @@ class LoopSearch if (!HasSingleEntryCycle()) { // There isn't actually a loop between TOP and BOTTOM + JITDUMP(" not single entry cycle\n"); return false; } if (!loopBlocks.IsMember(top->bbNum)) { // The "back-edge" we identified isn't actually part of the flow cycle containing ENTRY + JITDUMP(" top not in loop\n"); return false; } @@ -1828,6 +1840,7 @@ class LoopSearch if (!MakeCompactAndFindExits()) { // Unable to preserve well-formed loop during compaction. + JITDUMP(" can't compact\n"); return false; } @@ -1928,6 +1941,7 @@ class LoopSearch } else if (!comp->fgDominate(entry, block)) { + JITDUMP(" (find cycle) entry:" FMT_BB " does not dominate " FMT_BB "\n", entry->bbNum, block->bbNum); return false; } @@ -1960,6 +1974,8 @@ class LoopSearch } // There are multiple entries to this loop, don't consider it. + + JITDUMP(" (find cycle) multiple entry:" FMT_BB "\n", block->bbNum); return false; } @@ -1967,6 +1983,7 @@ class LoopSearch if (predBlock == entry) { // We have indeed found a cycle in the flow graph. + JITDUMP(" (find cycle) found cycle\n"); isFirstVisit = !foundCycle; foundCycle = true; assert(loopBlocks.IsMember(predBlock->bbNum)); @@ -2885,14 +2902,18 @@ bool Compiler::optIsLoopEntry(BasicBlock* block) const // // Notes: // For loopInd and all contained loops, ensures each loop top's back edges -// do not come from nested loops. +// only come from this loop. // // Will split top blocks and redirect edges if needed. // bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) { + // First canonicalize the loop. + // bool modified = optCanonicalizeLoop(loopInd); + // Then any children. + // for (unsigned char child = optLoopTable[loopInd].lpChild; // child != BasicBlock::NOT_IN_LOOP; // child = optLoopTable[child].lpSibling) @@ -2904,8 +2925,8 @@ bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) } //----------------------------------------------------------------------------- -// optCanonicalizeLoop: ensure that each loop top's back edges do not come -// from nested loops. +// optCanonicalizeLoop: ensure that each loop top's back edges come only from +// blocks in the same loop. // // Arguments: // loopInd - index of the loop to consider @@ -2913,21 +2934,110 @@ bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) // Returns: // true if flow changes were made // +// Notes: +// +// Back edges incident on loop top fall into one three groups: +// +// (1) Outer non-loop backedges (preds dominated by enter where pred is not in loop) +// (2) The canonical backedge (pred == bottom) +// (3) Nested loop backedges or nested non-loop backedges +// (preds dominated by enter, where pred is in loop, pred != bottom) +// +// We assume dominance has already been established by loop recognition (that is, +// anything classfied as a loop will have all backedges dominated by loop entry, +// so the only possible non-backedge predecessor of top will be head). +// +// We cannot check dominance here as the flow graph is being modified. +// +// If either set (1) or (3) is non-empty the loop is not canonical. +// +// This method will split the loop top into two or three blocks depending on +// whether (1) or (3) is non-empty, and redirect the edges accordingly. +// +// Loops are canoncalized outer to inner, so inner loops should never see outer loop +// non-backedges, as the parent loop canonicalization should have handled them. +// bool Compiler::optCanonicalizeLoop(unsigned char loopInd) { - // Is the top uniquely part of the current loop? + bool modified = false; + BasicBlock* const b = optLoopTable[loopInd].lpBottom; BasicBlock* const t = optLoopTable[loopInd].lpTop; + BasicBlock* const h = optLoopTable[loopInd].lpHead; + BasicBlock* const e = optLoopTable[loopInd].lpEntry; + + // Look for case (1) + // + bool doOuterCanon = false; - if (t->bbNatLoopNum == loopInd) + for (BasicBlock* const topPredBlock : t->PredBlocks()) { - return false; + const bool predIsInLoop = (t->bbNum <= topPredBlock->bbNum) && (topPredBlock->bbNum <= b->bbNum); + if (predIsInLoop || (topPredBlock == h)) + { + // no action needed + } + else + { + JITDUMP("in optCanonicalizeLoop: " FMT_LP " top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB ") %shas a non-loop backedge from " FMT_BB + "\n", + loopInd, t->bbNum, e->bbNum, b->bbNum, + doOuterCanon ? "also " : "", + topPredBlock->bbNum, + doOuterCanon ? "" : ": need to canonicalize non-loop backedges"); + doOuterCanon = true; + } } - JITDUMP("in optCanonicalizeLoop: " FMT_LP " has top " FMT_BB " (bottom " FMT_BB ") with natural loop number " FMT_LP - ": need to canonicalize\n", - loopInd, t->bbNum, optLoopTable[loopInd].lpBottom->bbNum, t->bbNatLoopNum); + if (doOuterCanon) + { + const bool didCanon = optCanonicalizeLoopCore(loopInd, LoopCanonicalizationOption::Outer); + assert(didCanon); + modified |= didCanon; + } - // Otherwise, the top of this loop is also part of a nested loop. + // Look for case (3) + // + // Outer canon should not update loop top. + // + assert(t == optLoopTable[loopInd].lpTop); + if (t->bbNatLoopNum != loopInd) + { + JITDUMP("in optCanonicalizeLoop: " FMT_LP " has top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB ") with natural loop number " FMT_LP + ": need to canonicalize nested inner loop backedges\n", + loopInd, t->bbNum, e->bbNum, b->bbNum, t->bbNatLoopNum); + + const bool didCanon = optCanonicalizeLoopCore(loopInd, LoopCanonicalizationOption::Current); + assert(didCanon); + modified |= didCanon; + } + + if (modified) + { + JITDUMP("Done canonicalizing " FMT_LP "\n\n", loopInd); + } + + return modified; +} + +//----------------------------------------------------------------------------- +// optCanonicalizeLoopCore: ensure that each loop top's back edges come do not +// come from outer/inner loops. +// +// Arguments: +// loopInd - index of the loop to consider +// option - which set of edges to move when canonicalizing +// +// Returns: +// true if flow changes were made +// +// Notes: +// option ::Outer retargets all backedges that do not come from loops in the block. +// option ::Current retargets the canonical backedge (from bottom) +// +bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizationOption option) +{ + // Otherwise, the top of this loop is also part of a nested loop or has + // non-loop backedges. // // Insert a new unique top for this loop. We must be careful to put this new // block in the correct EH region. Note that t->bbPrev might be in a different @@ -3003,9 +3113,10 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) // want to copy the EH region of the back edge, since that would create a block // outside of and disjoint with the "try" region of the back edge. However, to // simplify things, we disqualify this type of loop, so we should never see this here. - - BasicBlock* const h = optLoopTable[loopInd].lpHead; + // BasicBlock* const b = optLoopTable[loopInd].lpBottom; + BasicBlock* const t = optLoopTable[loopInd].lpTop; + BasicBlock* const h = optLoopTable[loopInd].lpHead; // The loop must be entirely contained within a single handler region. assert(BasicBlock::sameHndRegion(t, b)); @@ -3028,8 +3139,15 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) // If the bottom block is in the same "try" region, then we extend the EH // region. Otherwise, we add the new block outside the "try" region. - const bool extendRegion = BasicBlock::sameTryRegion(t, b); - BasicBlock* newT = fgNewBBbefore(BBJ_NONE, t, extendRegion); + // + const bool extendRegion = BasicBlock::sameTryRegion(t, b); + BasicBlock* const newT = fgNewBBbefore(BBJ_NONE, t, extendRegion); + + // Initially give newT the same weight as t; we will subtract from + // this for each edge that does not move from t to newT. + // + newT->inheritWeight(t); + if (!extendRegion) { // We need to set the EH region manually. Set it to be the same @@ -3037,73 +3155,92 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) newT->copyEHRegion(b); } + // NewT will be the target for the outer/current loop's backedge(s). + // + BlockToBlockMap* const blockMap = new (getAllocator(CMK_LoopOpt)) BlockToBlockMap(getAllocator(CMK_LoopOpt)); + blockMap->Set(t, newT); + // The new block can reach the same set of blocks as the old one, but don't try to reflect // that in its reachability set here -- creating the new block may have changed the BlockSet // representation from short to long, and canonicalizing loops is immediately followed by // a call to fgUpdateChangedFlowGraph which will recompute the reachability sets anyway. - // Redirect the "bottom" of the current loop to "newT". - BlockToBlockMap* const blockMap = new (getAllocator(CMK_LoopOpt)) BlockToBlockMap(getAllocator(CMK_LoopOpt)); - blockMap->Set(t, newT); - optRedirectBlock(b, blockMap); - JITDUMP("in optCanonicalizeLoop: redirecting bottom->top backedge " FMT_BB " -> " FMT_BB " to " FMT_BB " -> " FMT_BB - "\n", - b->bbNum, t->bbNum, b->bbNum, newT->bbNum); - - // Redirect non-loop preds of "t" to also go to "newT". Inner loops that also branch to "t" should continue - // to do so. However, there maybe be other predecessors from outside the loop nest that need to be updated - // to point to "newT". This normally wouldn't happen, since they too would be part of the loop nest. However, - // they might have been prevented from participating in the loop nest due to different EH nesting, or some - // other reason. - // - // Note that optRedirectBlock doesn't update the predecessors list. So, if the same 't' block is processed - // multiple times while canonicalizing multiple loop nests, we'll attempt to redirect a predecessor multiple times. - // This is ok, because after the first redirection, the topPredBlock branch target will no longer match the source - // edge of the blockMap, so nothing will happen. bool firstPred = true; for (BasicBlock* const topPredBlock : t->PredBlocks()) { - // Skip if topPredBlock is in the loop. - // Note that this uses block number to detect membership in the loop. We are adding blocks during - // canonicalization, and those block numbers will be new, and larger than previous blocks. However, we work - // outside-in, so we shouldn't encounter the new blocks at the loop boundaries, or in the predecessor lists. - if (t->bbNum <= topPredBlock->bbNum && topPredBlock->bbNum <= b->bbNum) + // We set profile weight of newT assuming all edges would + // be redirected there. So, if we don't redirect this edge, + // this is how much we'll have to adjust newT's weight. + // + weight_t weightAdjust = BB_ZERO_WEIGHT; + + if (option == LoopCanonicalizationOption::Current) { - // Note we've already redirected b->t above. + // Redirect the (one and only) true backedge of this loop. // - if (topPredBlock->bbNum != b->bbNum) + if (topPredBlock != b) + { + if ((topPredBlock != h) && topPredBlock->hasProfileWeight()) + { + // Note this may overstate the adjustment, if topPredBlock is BBJ_COND. + // + weightAdjust = topPredBlock->bbWeight; + } + } + else { - JITDUMP("in optCanonicalizeLoop: in-loop 'top' predecessor " FMT_BB " is in the range of " FMT_LP - " (" FMT_BB ".." FMT_BB "); not redirecting its bottom edge\n", - topPredBlock->bbNum, loopInd, t->bbNum, b->bbNum); + 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); } - continue; } - - JITDUMP("in optCanonicalizeLoop: redirect top predecessor " FMT_BB " to " FMT_BB "\n", topPredBlock->bbNum, - newT->bbNum); - optRedirectBlock(topPredBlock, blockMap); - - // When we have profile data then the 'newT' block will inherit topPredBlock profile weight - if (topPredBlock->hasProfileWeight()) + else if (option == LoopCanonicalizationOption::Outer) { - // This corrects an issue when the topPredBlock has a profile based weight + // Redirect non-loop preds of "t" to go to "newT". Inner loops that also branch to "t" should continue + // to do so. However, there maybe be other predecessors from outside the loop nest that need to be updated + // to point to "newT". This normally wouldn't happen, since they too would be part of the loop nest. However, + // they might have been prevented from participating in the loop nest due to different EH nesting, or some + // other reason. + // + // Skip if topPredBlock is in the loop. + // Note that this uses block number to detect membership in the loop. We are adding blocks during + // canonicalization, and those block numbers will be new, and larger than previous blocks. However, we work + // outside-in, so we shouldn't encounter the new blocks at the loop boundaries, or in the predecessor lists. // - if (firstPred) + if ((t->bbNum <= topPredBlock->bbNum) && (topPredBlock->bbNum <= b->bbNum)) { - JITDUMP("in optCanonicalizeLoop: block " FMT_BB " will inheritWeight from " FMT_BB "\n", newT->bbNum, - topPredBlock->bbNum); - - newT->inheritWeight(topPredBlock); - firstPred = false; + if (topPredBlock->hasProfileWeight()) + { + // Note this may overstate the adjustment, if topPredBlock is BBJ_COND. + // + weightAdjust = topPredBlock->bbWeight; + } } else { - JITDUMP("in optCanonicalizeLoop: block " FMT_BB " will also contribute to the weight of " FMT_BB "\n", - newT->bbNum, topPredBlock->bbNum); + JITDUMP("in optCanonicalizeLoop (outer): redirect %s->top %sedge " FMT_BB " -> " FMT_BB " to " FMT_BB " -> " FMT_BB "\n", + topPredBlock == h ? "head" : "nonloop", topPredBlock == h ? "" : "back", topPredBlock->bbNum, t->bbNum, + topPredBlock->bbNum, newT->bbNum); + optRedirectBlock(topPredBlock, blockMap); + } + } + else + { + unreached(); + } - weight_t newWeight = newT->getBBWeight(this) + topPredBlock->getBBWeight(this); - newT->setBBProfileWeight(newWeight); + if (weightAdjust > BB_ZERO_WEIGHT) + { + JITDUMP("in optCanonicalizeLoop: removing block " FMT_BB " weight " FMT_WT " from " FMT_BB "\n", + topPredBlock->bbNum, weightAdjust, newT->bbNum); + + if (newT->bbWeight >= weightAdjust) + { + newT->setBBProfileWeight(newT->bbWeight - weightAdjust); + } + else if (newT->bbWeight > BB_ZERO_WEIGHT) + { + newT->setBBProfileWeight(BB_ZERO_WEIGHT); } } } @@ -3111,39 +3248,59 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) assert(h->bbNext == newT); assert(newT->bbNext == t); - // If loopInd is a do-while loop (top == entry), update entry as well. + // With the Option::Current we are changing which block is loop top. + // Make sutable updates. // - BasicBlock* const origE = optLoopTable[loopInd].lpEntry; - if (optLoopTable[loopInd].lpTop == origE) + if (option == LoopCanonicalizationOption::Current) { - optLoopTable[loopInd].lpEntry = newT; - } - optLoopTable[loopInd].lpTop = newT; + JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop %d.\n", newT->bbNum, + loopInd); - newT->bbNatLoopNum = loopInd; + optLoopTable[loopInd].lpTop = newT; + newT->bbNatLoopNum = loopInd; - JITDUMP("in optCanonicalizeLoop: made new block " FMT_BB " [%p] the new unique top of loop %d.\n", newT->bbNum, - dspPtr(newT), loopInd); - - // If any loops nested in "loopInd" have the same head and entry as "loopInd", - // it must be the case that they were also do-while's (since "h" fell through to the entry). - // The new node "newT" becomes the head of such loops. - // - for (unsigned char childLoop = optLoopTable[loopInd].lpChild; // - childLoop != BasicBlock::NOT_IN_LOOP; // - childLoop = optLoopTable[childLoop].lpSibling) - { - if (optLoopTable[childLoop].lpEntry == origE && optLoopTable[childLoop].lpHead == h && - newT->bbJumpKind == BBJ_NONE && newT->bbNext == origE) + // If loopInd was a do-while loop (top == entry), update entry, as well. + // + BasicBlock* const origE = optLoopTable[loopInd].lpEntry; + if (origE == t) { - optUpdateLoopHead(childLoop, h, newT); + JITDUMP("updating entry of " FMT_LP " to " FMT_BB "\n", loopInd, newT->bbNum); + optLoopTable[loopInd].lpEntry = newT; + } - // Fix pred list here, so when we walk preds of child loop tops - // we see the right blocks. - // - fgReplacePred(optLoopTable[childLoop].lpTop, h, newT); + // If any loops nested in "loopInd" have the same head and entry as "loopInd", + // it must be the case that they were do-while's (since "h" fell through to the entry). + // The new node "newT" becomes the head of such loops. + for (unsigned char childLoop = optLoopTable[loopInd].lpChild; // + childLoop != BasicBlock::NOT_IN_LOOP; // + childLoop = optLoopTable[childLoop].lpSibling) + { + if ((optLoopTable[childLoop].lpEntry == origE) && (optLoopTable[childLoop].lpHead == h) && + (newT->bbJumpKind == BBJ_NONE) && (newT->bbNext == origE)) + { + optUpdateLoopHead(childLoop, h, newT); + + // Fix pred list here, so when we walk preds of child loop tops + // we see the right blocks. + // + fgReplacePred(optLoopTable[childLoop].lpTop, h, newT); + } } } + else if (option == LoopCanonicalizationOption::Outer) + { + JITDUMP("in optCanonicalizeLoop (outer): " FMT_BB " is outside of loop " FMT_LP "\n", newT->bbNum, + loopInd); + + // If we are lifting outer backeges, then newT belongs to our parent loop + // + newT->bbNatLoopNum = optLoopTable[loopInd].lpParent; + + // newT is now the header of this loop + // + optUpdateLoopHead(loopInd, h, newT); + } + return true; } From ec6985c100a9316c8e3fa4b6258bdf5019ef014d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Jun 2022 10:10:24 -0700 Subject: [PATCH 2/4] formatting --- src/coreclr/jit/optimizer.cpp | 82 +++++++++++++++++------------------ 1 file changed, 39 insertions(+), 43 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 7c359a269255ed..bb42f855526207 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1725,14 +1725,13 @@ class LoopSearch // Thus, we have to be very careful and after entry discovery check that it is indeed // the only place we enter the loop (especially for non-reducible flow graphs). - JITDUMP("FindLoop: checking head:" FMT_BB " top:" FMT_BB " bottom:" FMT_BB "\n", - head->bbNum, top->bbNum, bottom->bbNum); + JITDUMP("FindLoop: checking head:" FMT_BB " top:" FMT_BB " bottom:" FMT_BB "\n", head->bbNum, top->bbNum, + bottom->bbNum); if (top->bbNum > bottom->bbNum) // is this a backward edge? (from BOTTOM to TOP) { // Edge from BOTTOM to TOP is not a backward edge - JITDUMP(" " FMT_BB "->" FMT_BB " is not a backedge\n", - bottom->bbNum, top->bbNum); + JITDUMP(" " FMT_BB "->" FMT_BB " is not a backedge\n", bottom->bbNum, top->bbNum); return false; } @@ -1740,8 +1739,7 @@ class LoopSearch { // Not a true back-edge; bottom is a block added to reconnect fall-through during // loop processing, so its block number does not reflect its position. - JITDUMP(" " FMT_BB "->" FMT_BB " is not a true backedge\n", - bottom->bbNum, top->bbNum); + JITDUMP(" " FMT_BB "->" FMT_BB " is not a true backedge\n", bottom->bbNum, top->bbNum); return false; } @@ -1753,7 +1751,6 @@ class LoopSearch return false; } - // The presence of a "back edge" is an indication that a loop might be present here. // // Definition: A loop is: @@ -1975,7 +1972,7 @@ class LoopSearch // There are multiple entries to this loop, don't consider it. - JITDUMP(" (find cycle) multiple entry:" FMT_BB "\n", block->bbNum); + JITDUMP(" (find cycle) multiple entry:" FMT_BB "\n", block->bbNum); return false; } @@ -2937,10 +2934,10 @@ bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) // Notes: // // Back edges incident on loop top fall into one three groups: -// +// // (1) Outer non-loop backedges (preds dominated by enter where pred is not in loop) // (2) The canonical backedge (pred == bottom) -// (3) Nested loop backedges or nested non-loop backedges +// (3) Nested loop backedges or nested non-loop backedges // (preds dominated by enter, where pred is in loop, pred != bottom) // // We assume dominance has already been established by loop recognition (that is, @@ -2954,16 +2951,16 @@ bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) // This method will split the loop top into two or three blocks depending on // whether (1) or (3) is non-empty, and redirect the edges accordingly. // -// Loops are canoncalized outer to inner, so inner loops should never see outer loop +// Loops are canoncalized outer to inner, so inner loops should never see outer loop // non-backedges, as the parent loop canonicalization should have handled them. // bool Compiler::optCanonicalizeLoop(unsigned char loopInd) { - bool modified = false; - BasicBlock* const b = optLoopTable[loopInd].lpBottom; - BasicBlock* const t = optLoopTable[loopInd].lpTop; - BasicBlock* const h = optLoopTable[loopInd].lpHead; - BasicBlock* const e = optLoopTable[loopInd].lpEntry; + bool modified = false; + BasicBlock* const b = optLoopTable[loopInd].lpBottom; + BasicBlock* const t = optLoopTable[loopInd].lpTop; + BasicBlock* const h = optLoopTable[loopInd].lpHead; + BasicBlock* const e = optLoopTable[loopInd].lpEntry; // Look for case (1) // @@ -2978,12 +2975,10 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) } else { - JITDUMP("in optCanonicalizeLoop: " FMT_LP " top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB ") %shas a non-loop backedge from " FMT_BB - "\n", - loopInd, t->bbNum, e->bbNum, b->bbNum, - doOuterCanon ? "also " : "", - topPredBlock->bbNum, - doOuterCanon ? "" : ": need to canonicalize non-loop backedges"); + JITDUMP("in optCanonicalizeLoop: " FMT_LP " top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB + ") %shas a non-loop backedge from " FMT_BB "\n", + loopInd, t->bbNum, e->bbNum, b->bbNum, doOuterCanon ? "also " : "", topPredBlock->bbNum, + doOuterCanon ? "" : ": need to canonicalize non-loop backedges"); doOuterCanon = true; } } @@ -3002,9 +2997,9 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) assert(t == optLoopTable[loopInd].lpTop); if (t->bbNatLoopNum != loopInd) { - JITDUMP("in optCanonicalizeLoop: " FMT_LP " has top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB ") with natural loop number " FMT_LP - ": need to canonicalize nested inner loop backedges\n", - loopInd, t->bbNum, e->bbNum, b->bbNum, t->bbNatLoopNum); + JITDUMP("in optCanonicalizeLoop: " FMT_LP " has top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB + ") with natural loop number " FMT_LP ": need to canonicalize nested inner loop backedges\n", + loopInd, t->bbNum, e->bbNum, b->bbNum, t->bbNatLoopNum); const bool didCanon = optCanonicalizeLoopCore(loopInd, LoopCanonicalizationOption::Current); assert(didCanon); @@ -3140,8 +3135,8 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati // If the bottom block is in the same "try" region, then we extend the EH // region. Otherwise, we add the new block outside the "try" region. // - const bool extendRegion = BasicBlock::sameTryRegion(t, b); - BasicBlock* const newT = fgNewBBbefore(BBJ_NONE, t, extendRegion); + const bool extendRegion = BasicBlock::sameTryRegion(t, b); + BasicBlock* const newT = fgNewBBbefore(BBJ_NONE, t, extendRegion); // Initially give newT the same weight as t; we will subtract from // this for each edge that does not move from t to newT. @@ -3187,10 +3182,11 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati weightAdjust = topPredBlock->bbWeight; } } - else + else { - 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); + 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); } } @@ -3198,7 +3194,8 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati { // Redirect non-loop preds of "t" to go to "newT". Inner loops that also branch to "t" should continue // to do so. However, there maybe be other predecessors from outside the loop nest that need to be updated - // to point to "newT". This normally wouldn't happen, since they too would be part of the loop nest. However, + // to point to "newT". This normally wouldn't happen, since they too would be part of the loop nest. + // However, // they might have been prevented from participating in the loop nest due to different EH nesting, or some // other reason. // @@ -3210,7 +3207,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati if ((t->bbNum <= topPredBlock->bbNum) && (topPredBlock->bbNum <= b->bbNum)) { if (topPredBlock->hasProfileWeight()) - { + { // Note this may overstate the adjustment, if topPredBlock is BBJ_COND. // weightAdjust = topPredBlock->bbWeight; @@ -3218,9 +3215,10 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati } else { - JITDUMP("in optCanonicalizeLoop (outer): redirect %s->top %sedge " FMT_BB " -> " FMT_BB " to " FMT_BB " -> " FMT_BB "\n", - topPredBlock == h ? "head" : "nonloop", topPredBlock == h ? "" : "back", topPredBlock->bbNum, t->bbNum, - topPredBlock->bbNum, newT->bbNum); + JITDUMP("in optCanonicalizeLoop (outer): redirect %s->top %sedge " FMT_BB " -> " FMT_BB " to " FMT_BB + " -> " FMT_BB "\n", + topPredBlock == h ? "head" : "nonloop", topPredBlock == h ? "" : "back", topPredBlock->bbNum, + t->bbNum, topPredBlock->bbNum, newT->bbNum); optRedirectBlock(topPredBlock, blockMap); } } @@ -3232,8 +3230,8 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati if (weightAdjust > BB_ZERO_WEIGHT) { JITDUMP("in optCanonicalizeLoop: removing block " FMT_BB " weight " FMT_WT " from " FMT_BB "\n", - topPredBlock->bbNum, weightAdjust, newT->bbNum); - + topPredBlock->bbNum, weightAdjust, newT->bbNum); + if (newT->bbWeight >= weightAdjust) { newT->setBBProfileWeight(newT->bbWeight - weightAdjust); @@ -3253,11 +3251,10 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati // if (option == LoopCanonicalizationOption::Current) { - JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop %d.\n", newT->bbNum, - loopInd); + JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop %d.\n", newT->bbNum, loopInd); optLoopTable[loopInd].lpTop = newT; - newT->bbNatLoopNum = loopInd; + newT->bbNatLoopNum = loopInd; // If loopInd was a do-while loop (top == entry), update entry, as well. // @@ -3279,7 +3276,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati (newT->bbJumpKind == BBJ_NONE) && (newT->bbNext == origE)) { optUpdateLoopHead(childLoop, h, newT); - + // Fix pred list here, so when we walk preds of child loop tops // we see the right blocks. // @@ -3289,8 +3286,7 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati } else if (option == LoopCanonicalizationOption::Outer) { - JITDUMP("in optCanonicalizeLoop (outer): " FMT_BB " is outside of loop " FMT_LP "\n", newT->bbNum, - loopInd); + JITDUMP("in optCanonicalizeLoop (outer): " FMT_BB " is outside of loop " FMT_LP "\n", newT->bbNum, loopInd); // If we are lifting outer backeges, then newT belongs to our parent loop // From c9b6f46826ee8a45ed3a9206e37b4f6c1817ffc7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Jun 2022 12:34:58 -0700 Subject: [PATCH 3/4] review feedback --- src/coreclr/jit/optimizer.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index bb42f855526207..1a0657458d25bf 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2935,13 +2935,13 @@ bool Compiler::optCanonicalizeLoopNest(unsigned char loopInd) // // Back edges incident on loop top fall into one three groups: // -// (1) Outer non-loop backedges (preds dominated by enter where pred is not in loop) +// (1) Outer non-loop backedges (preds dominated by entry where pred is not in loop) // (2) The canonical backedge (pred == bottom) // (3) Nested loop backedges or nested non-loop backedges -// (preds dominated by enter, where pred is in loop, pred != bottom) +// (preds dominated by entry, where pred is in loop, pred != bottom) // // We assume dominance has already been established by loop recognition (that is, -// anything classfied as a loop will have all backedges dominated by loop entry, +// anything classified as a loop will have all backedges dominated by loop entry, // so the only possible non-backedge predecessor of top will be head). // // We cannot check dominance here as the flow graph is being modified. @@ -2976,7 +2976,7 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd) else { JITDUMP("in optCanonicalizeLoop: " FMT_LP " top " FMT_BB " (entry " FMT_BB " bottom " FMT_BB - ") %shas a non-loop backedge from " FMT_BB "\n", + ") %shas a non-loop backedge from " FMT_BB "%s\n", loopInd, t->bbNum, e->bbNum, b->bbNum, doOuterCanon ? "also " : "", topPredBlock->bbNum, doOuterCanon ? "" : ": need to canonicalize non-loop backedges"); doOuterCanon = true; @@ -3247,11 +3247,11 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati assert(newT->bbNext == t); // With the Option::Current we are changing which block is loop top. - // Make sutable updates. + // Make suitable updates. // if (option == LoopCanonicalizationOption::Current) { - JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop %d.\n", newT->bbNum, loopInd); + JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop " FMT_LP "\n", newT->bbNum, loopInd); optLoopTable[loopInd].lpTop = newT; newT->bbNatLoopNum = loopInd; From 6a2c3c2196c908f540227fdb71b8456d8386814b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Jun 2022 12:48:48 -0700 Subject: [PATCH 4/4] format --- src/coreclr/jit/optimizer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 1a0657458d25bf..211bd9466be42d 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -3251,7 +3251,8 @@ bool Compiler::optCanonicalizeLoopCore(unsigned char loopInd, LoopCanonicalizati // if (option == LoopCanonicalizationOption::Current) { - JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop " FMT_LP "\n", newT->bbNum, loopInd); + JITDUMP("in optCanonicalizeLoop (current): " FMT_BB " is now the top of loop " FMT_LP "\n", newT->bbNum, + loopInd); optLoopTable[loopInd].lpTop = newT; newT->bbNatLoopNum = loopInd;