<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Justin,<div class=""><br class=""></div><div class="">Thanks for taking a look, please find my comments below. I’ll update the patch shortly!</div><div class=""><br class=""></div><div class="">Michael<br class=""><div><blockquote type="cite" class=""><div class="">On Feb 3, 2016, at 11:56 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" class="">mail@justinbogner.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Michael Zolotukhin <</span><a href="mailto:mzolotukhin@apple.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">mzolotukhin@apple.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">> writes:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">mzolotukhin created this revision.<br class="">mzolotukhin added reviewers: chandlerc, hfinkel, dexonsmith, bogner, joker.eph.<br class="">mzolotukhin added a subscriber: llvm-commits.<br class=""><br class="">In r255133 (reapplied r253126) we started to avoid redundant<br class="">recomputation of LCSSA after loop-unrolling. This patch moves one step<br class="">further in this direction - now we can avoid it for much wider range<br class="">of loops, as we start to look at IR and try to figure out if the<br class="">transformation actually breaks LCSSA phis or makes it necessary to<br class="">insert new ones.<br class=""><br class="">In future we might go even further and try to fix LCSSA in-place<br class="">rather than rebuilding it, but I'm not quite sure yet that it's always<br class="">possible (and computationally cheaper). Anyway, this patch seems to be<br class="">aligned with that direction.<br class=""><br class="">One of the most important use-cases that previous implementation<br class="">didn't handle is loops with calls that might throw an exception. Such<br class="">loops have exits from entire loop nest, but we still don't need to<br class="">recompute LCSSA after unrolling, as such exits usually don't contain<br class="">LCSSA phis.<br class=""><br class="">The patch was tested on LNT testsuite + SPECS, neither failures nor<br class="">significant compile time changes were spotted.<br class=""><br class=""><a href="http://reviews.llvm.org/D16838" class="">http://reviews.llvm.org/D16838</a><br class=""><br class="">Files:<br class=""> lib/Transforms/Utils/LoopUnroll.cpp<br class=""><br class="">Index: lib/Transforms/Utils/LoopUnroll.cpp<br class="">===================================================================<br class="">--- lib/Transforms/Utils/LoopUnroll.cpp<br class="">+++ lib/Transforms/Utils/LoopUnroll.cpp<br class="">@@ -218,10 +218,16 @@<br class=""> bool CompletelyUnroll = Count == TripCount;<br class=""> SmallVector<BasicBlock *, 4> ExitBlocks;<br class=""> L->getExitBlocks(ExitBlocks);<br class="">- Loop *ParentL = L->getParentLoop();<br class="">- bool AllExitsAreInsideParentLoop = !ParentL ||<br class="">- std::all_of(ExitBlocks.begin(), ExitBlocks.end(),<br class="">- [&](BasicBlock *BB) { return ParentL->contains(BB); });<br class="">+<br class="">+ // Go through all exits of L and see if there are any phi-nodes there. We just<br class="">+ // conservatively assume that they're inserted to preserve LCSSA form, which<br class="">+ // means that complete unrolling might break this form. We need to either fix<br class="">+ // it in-place after the transformation, or entirely rebuild LCSSA. TODO: For<br class="">+ // now we just recompute LCSSA for the outer loop, but it should be possible<br class="">+ // to fix it in-place.<br class="">+ bool NeedToFixLCSSA = CompletelyUnroll &&<br class="">+ std::any_of(ExitBlocks.begin(), ExitBlocks.end(),<br class="">+ [&](BasicBlock *BB) { return isa<PHINode>(BB->begin()); });<br class=""><br class=""> // We assume a run-time trip count if the compiler cannot<br class=""> // figure out the loop trip count and the unroll-runtime<br class="">@@ -308,6 +314,7 @@<br class=""> LoopBlocksDFS::RPOIterator BlockBegin = DFS.beginRPO();<br class=""> LoopBlocksDFS::RPOIterator BlockEnd = DFS.endRPO();<br class=""><br class="">+ std::vector<BasicBlock*> UnrolledLoopBlocks = L->getBlocks();<br class=""> for (unsigned It = 1; It != Count; ++It) {<br class=""> std::vector<BasicBlock*> NewBlocks;<br class=""> SmallDenseMap<const Loop *, Loop *, 4> NewLoops;<br class="">@@ -387,6 +394,7 @@<br class=""> Latches.push_back(New);<br class=""><br class=""> NewBlocks.push_back(New);<br class="">+ UnrolledLoopBlocks.push_back(New);<br class=""> }<br class=""><br class=""> // Remap all instructions in the most recent iteration<br class="">@@ -476,8 +484,12 @@<br class=""> if (Term->isUnconditional()) {<br class=""> BasicBlock *Dest = Term->getSuccessor(0);<br class=""> if (BasicBlock *Fold = FoldBlockIntoPredecessor(Dest, LI, SE,<br class="">- ForgottenLoops))<br class="">+ ForgottenLoops)) {<br class=""> std::replace(Latches.begin(), Latches.end(), Dest, Fold);<br class="">+ UnrolledLoopBlocks.erase(std::remove(UnrolledLoopBlocks.begin(),<br class="">+ UnrolledLoopBlocks.end(), Dest),<br class="">+ UnrolledLoopBlocks.end());<br class="">+ }<br class=""> }<br class=""> }<br class=""><br class="">@@ -530,15 +542,38 @@<br class=""> if (CompletelyUnroll)<br class=""> LI->markAsRemoved(L);<br class=""><br class="">+ // After complete unrolling most of the blocks should be contained in OuterL.<br class="">+ // However, some of them might happen to be out of OuterL (e.g. if they<br class="">+ // precede a loop exit). In this case we might need to insert PHI nodes in<br class="">+ // order to preserve LCSSA form.<br class="">+ // We don't need to check this if we already know that we need to fix LCSSA<br class="">+ // form.<br class="">+ // TODO: For now we just recompute LCSSA for the outer loop in this case, but<br class="">+ // it should be possible to fix it in-place.<br class="">+ if (OuterL && CompletelyUnroll && !NeedToFixLCSSA) {<br class="">+ for (unsigned i = 0; i < UnrolledLoopBlocks.size(); ++i) {<br class="">+ BasicBlock *BB = UnrolledLoopBlocks[i];<br class="">+ if (LI->getLoopFor(BB) == OuterL)<br class="">+ continue;<br class="">+ for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I) {<br class="">+ for (unsigned OpI = 0, OpE = I->getNumOperands(); OpI < OpE; ++OpI) {<br class="">+ if (auto Def = dyn_cast<Instruction>(I->getOperand(OpI)))<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I think you can use ranged for for these loops:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> for (Instruction &Inst : BB) {</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> for (Use &U : BB.operands()) {</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> if (auto Def = dyn_cast<Instruction>(U))</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> …</span></div></blockquote>Good point. Probably, even all_of/any_of would work here, I’ll fix this.<br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">+ if (LI->getLoopFor(Def->getParent()) == OuterL)<br class="">+ NeedToFixLCSSA = true;<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">We should be able to break out early here, no?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote>Definitely, thanks!</div><div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">+ }<br class="">+ }<br class="">+ }<br class="">+ }<br class="">+<br class=""> // If we have a pass and a DominatorTree we should re-simplify impacted loops<br class=""> // to ensure subsequent analyses can rely on this form. We want to simplify<br class=""> // at least one layer outside of the loop that was unrolled so that any<br class=""> // changes to the parent loop exposed by the unrolling are considered.<br class=""> if (DT) {<br class=""> if (!OuterL && !CompletelyUnroll)<br class=""> OuterL = L;<br class=""> if (OuterL) {<br class="">- bool Simplified = simplifyLoop(OuterL, DT, LI, SE, AC, PreserveLCSSA);<br class="">+ simplifyLoop(OuterL, DT, LI, SE, AC, PreserveLCSSA);<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Can simplifyLoop return false when NeedToFixLCSSA = true? If so, do we</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">still need to fix LCSSA?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote>Previously I, for some reason, thought that simplification might break LCSSA, so we have to rebuild it after it, but this time I haven’t found such a case. I’ll examine what we can do there and update the patch accordingly if needed.<br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""> // LCSSA must be performed on the outermost affected loop. The unrolled<br class=""> // loop's last loop latch is guaranteed to be in the outermost loop after<br class="">@@ -548,7 +583,7 @@<br class=""> while (OuterL->getParentLoop() != LatchLoop)<br class=""> OuterL = OuterL->getParentLoop();<br class=""><br class="">- if (CompletelyUnroll && (!AllExitsAreInsideParentLoop || Simplified))<br class="">+ if (CompletelyUnroll && NeedToFixLCSSA)<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">CompletelyUnroll is kind of redundant here, since there's no path where</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">we set NeedToFixLCSSA = true where we didn't check CompletelyUnroll</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">already.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote>True, will fix!</div><div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> formLCSSARecursively(*OuterL, *DT, LI, SE);<br class=""> else<br class=""> assert(OuterL->isLCSSAForm(*DT) &&</blockquote></div></blockquote></div><br class=""></div></body></html>