<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=""><br class=""><div><blockquote type="cite" class=""><div class="">On Nov 10, 2015, at 1:12 PM, 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="">Hi Duncan,<br class=""><br class=""><blockquote type="cite" class="">On Nov 10, 2015, at 9:45 AM, Duncan P. N. Exon Smith<br class=""><<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On 2015-Nov-09, at 20:54, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com" class="">mzolotukhin@apple.com</a>> wrote:<br class=""><br class="">mzolotukhin created this revision.<br class="">mzolotukhin added reviewers: chandlerc, hfinkel, dexonsmith,<br class="">bogner, joker.eph.<br class="">mzolotukhin added a subscriber: llvm-commits.<br class=""><br class="">Currently we always recompute LCSSA for outer loops after unrolling an<br class="">inner loop. That leads to compile time problem when we have big loop<br class="">nests, and we can solve it by avoiding unnecessary work. For instance,<br class="">if w eonly do partial unrolling, we don't break LCSSA, so we don't need<br class="">to rebuild it. Also, if all exits from the inner loop are inside the<br class="">enclosing loop, then complete unrolling won't break LCSSA either.<br class=""><br class="">I replaced unconditional LCSSA recomputation with conditional recomputation +<br class="">unconditional assert and added several tests, which were failing when I<br class="">experimented with it.<br class=""><br class="">Soon I plan to follow up with a similar patch for recalculation of dominators<br class="">tree.<br class=""><br class=""><a href="http://reviews.llvm.org/D14526" class="">http://reviews.llvm.org/D14526</a><br class=""><br class="">Files:<br class="">lib/Transforms/Utils/LoopUnroll.cpp<br class="">test/Transforms/LoopUnroll/rebuild_lcssa.ll<br class=""><br class=""><D14526.39782.patch><br class=""></blockquote><br class="">This makes sense to me, although I don't know the loop algorithms too well<br class="">so maybe someone else can confirm. A few nitpicks below.<br class=""><br class=""><blockquote type="cite" 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="">@@ -221,6 +221,16 @@<br class=""><br class=""> // Are we eliminating the loop control altogether?<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 = true;<br class="">+ for (auto BB : ExitBlocks) {<br class=""></blockquote><br class="">This would be clearer as `const BasicBlock *BB`, but should at least be<br class="">`const auto *BB` to make clear to readers that there are no copies going<br class="">on.<br class=""></blockquote>Makes sense.<br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class="">+ if (LI->getLoopFor(BB) != ParentL) {<br class="">+ AllExitsAreInsideParentLoop = false;<br class="">+ break;<br class="">+ }<br class="">+ }<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="">@@ -554,7 +564,11 @@<br class=""> while (OuterL->getParentLoop() != LatchLoop)<br class=""> OuterL = OuterL->getParentLoop();<br class=""><br class="">- formLCSSARecursively(*OuterL, *DT, LI, SE);<br class="">+ if (CompletelyUnroll && !AllExitsAreInsideParentLoop)<br class="">+ formLCSSARecursively(*OuterL, *DT, LI, SE);<br class="">+<br class="">+ assert(OuterL->isLCSSAForm(*DT) &&<br class=""></blockquote><br class="">Is this check cheap? If not, it seems wasteful to do it when we've just<br class="">called `formLCSSARecursively()`. Can you put this in an `else`, or do<br class="">an early continue inside the `if`?<br class=""></blockquote>I thought it was fine to have even expensive checks under assert - isn’t it?<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="">It's "fine", but if it's easy to avoid it we might as well. No sense</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="">making the asserts build slower for no reason.</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>Fair enough. I moved this assertion under the ‘else' in the updated patch, other remarks should also be addressed.</div><div><br class=""></div><div>Michael<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=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class="">+ "Must have loops in LCSSA form to after loop-unroll.");<br class=""></blockquote><br class="">s/to after/after/<br class=""></blockquote>Thanks!<br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class=""> }<br class=""> }</blockquote></blockquote></blockquote></div></blockquote></div><br class=""></body></html>