<html><head><base href="x-msg://1797/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Nov 9, 2011, at 2:41 PM, Brendon Cahoon wrote:</div><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div lang="EN-US" link="blue" vlink="purple" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div class="WordSection1" style="page: WordSection1; "><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Hi Andy,<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Thanks for the reply. Much appreciated.<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I agree that the optimization may not help every target, and can cause some performance problems. Target specific information would be a huge help. Since our target is a VLIW, we have found the pass to be useful on several SPEC2K and EEMBC benchmarks. We also see some improvements on ARM for some of the programs in those benchmark suites. But, we have seen some performance regressions especially on ARM due to an increase in register spills, which we’re investigating. I haven’t done any significant performance analysis for x86 with the patch though.<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I *<b>think</b>* I’m updating the preserved info correctly. I believe the dominator information gets recomputed by the existing loop unrolling code. I will run tests with the –verify-dom-info and –verify-loop-info flags though. I hadn’t tried that. I’ll also try your suggestions about disabling the normal unrolling code for compile-time loops, and I can run some more tests when changing loop count.<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Thanks again,<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">-- Brendon<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 10.5pt; font-family: Consolas; color: rgb(31, 73, 125); ">--<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 10.5pt; font-family: Consolas; color: rgb(31, 73, 125); ">Qualcomm Innovation Center, Inc is a member of Code Aurora Forum<o:p></o:p></span></div></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div><div style="border-right-style: none; border-bottom-style: none; border-left-style: none; border-width: initial; border-color: initial; border-top-style: solid; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding-top: 3pt; padding-right: 0in; padding-bottom: 0in; padding-left: 0in; "><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; "><span class="Apple-converted-space"> </span>Andrew Trick [mailto:atrick@apple.com]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Wednesday, November 09, 2011 12:36 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Brendon Cahoon<br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [llvm-commits] [PATCH] Loop unrolling for run-time trip counts<o:p></o:p></span></div></div></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div><div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">On Nov 6, 2011, at 3:22 PM, Brendon Cahoon wrote:<o:p></o:p></div></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt; "><div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Hi,<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">This patch contains code to unroll loops that contain a run-time trip count. It extends the existing code, in the LoopUnroll and LoopUnrollPass classes, that unrolls loops with compile-time trip counts.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">The ability to unroll loops with run-time trip counts using this patch is turned off by default. To enable the transformation, I added an option, –unroll-runtime. It’s probably best to keep it disabled for now since some programs may degrade in performance with the option enabled. We’re hoping that some more tuning will help minimize any performance regressions. Of course, we do see performance improvements as well.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">I tested llvm with the patch and it passes ‘make check-all’ and there are no regressions in test-suite when using the ‘simple’ test target. I have also enabled the option and tested using test-suite, and there are no regressions with the option enabled. With the option enabled, there will be some errors with the tests when running ‘make check-all’ for two reasons. The existing tests assume that there is no loop unrolling for run-time trip counts, and my implementation requires that loop simplify is run afterward (I am planning on fixing this issue). I’ve also tested the code on a different test suite for ARM and our soon to be added backend.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">This implementation works by using the existing loop unrolling code to actually unroll the loop by some unroll factor (the default is 8 iterations). This patch generates code prior to the loop to compute the number of extra iterations to execute before entering the unrolled loop. The number of extra iterations is the run-time trip count modulo the unroll factor. We generate code to check the number of extra iterations and branch to the extra copies of the loop body that execute the extra iterations before entering the unrolled loop. We generate an if-then-else sequence, which may get converted to a switch statement by LLVM. The patch generates ‘unroll factor – 1’ copies of the loop body prior to the loop to execute these extra iterations. <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">This implementation only allow unroll factors that are a power to 2 to reduce the cost of computing the number of extra iterations. There are other limitations in implementation including only allowing loops with a single exit that occurs in the latch block. There is certainly some room for improving the code, but it works and improves performance on some actual benchmarks.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">The patch includes changes to the following files:<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> lib/Transforms/Scalar/LoopUnrollPass.cpp<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> lib/Transfoms/Utils/LoopUnroll.cpp<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> include/llvm/Transforms/Utils/UnrollLoop.h<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">And new files in the test directory: test/Transforms/LoopUnroll/runtime-loop[1-3].ll<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">I appreciate any comments, questions, etc. on the code.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Thanks,<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">-- Brendon Cahoon<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 10.5pt; font-family: Consolas; ">--</span><span style="font-size: 11pt; font-family: Calibri, sans-serif; "><o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 10.5pt; font-family: Consolas; ">Qualcomm Innovation Center, Inc is a member of Code Aurora Forum</span><span style="font-size: 11pt; font-family: Calibri, sans-serif; "><o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div></div></blockquote><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Brendon,<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Thanks for the patch. I see no problem adding this as an experimental framework given the testing you've done and unit tests you've provided. I suspect the optimization will be useful on some targets if branch profiles are available, logic is added to detect optimizations that can be exploited by unrolling, and/or target-specific heuristics can be queried to determine which loops may benefit.<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Before I dig into code review, let me ask a high level question. Creating pre/post loops complicates the CFG. Are you sure you're updating the preserved passes properly, including DominatorTree and LoopInfo. Please run the test suite with forced runtime unrolling plus -verify-dom-info and -verify-loop-info. To further stress your framework, make sure constant trip unrolling is disabled in your runs, so your unroller processes these too. I would also try with a small unroll (e.g. 2) count to stress the loop body and a large count to stress prolog/epilog.</div></div></div></div></span></blockquote></div><br><div><div>For the purpose of code review, I'll ignore the issue of</div><div>unroll-runtime profitability, which is obviously left for later. I</div><div>will say that this is only half the framework because to achieve</div><div>data-alignment we would need to emit an epilog as well. Also, I</div><div>misread your original message, and assumed you were adding a prolog</div><div>*loop*. That would be nice for targets that care about code</div><div>size. There are cases when unrolling the main loop can be done without</div><div>increasing the size of the loop body. Your design would not always be</div><div>well suited for those cases. On the other hand, your design is a much</div><div>simpler CFG transform. We have less SSA/LoopInfo updating to worry</div><div>about this way.</div><div><br></div><div>Never mind my earlier comment about DominatorTree. I forgot we always</div><div>recompute it. You're welcome to fix that :)</div><div><br></div><div>It does seem like your transform should preserve LoopSimplify. I'm</div><div>guessing you just need a unique loop exit block--just split the</div><div>exit. Can you please do that?</div><div><br></div><div>In general, your implementation looks quite nice.</div><div><br></div><div>I am concerned that you're doubling the size of LoopUnroll.cpp for a</div><div>feature that most people won't need to look at. I'd be ok with you</div><div>creating a new .cpp file. Or at least make it blatantly obvious that</div><div>all the new code is specific to runtime-unroll. For example, you can</div><div>add function-level comments like "Support for -runtime-unroll", and</div><div>code sections like:</div><div>//===----------------------------------------------------------------------===//</div><div>// Implementation of -runtime-unroll.</div><div>//===----------------------------------------------------------------------===//</div><div><your code></div><div>//===----------------------------------------------------------------------===//</div><div>// Driver for loop unrolling transformations.</div><div>//===----------------------------------------------------------------------===//</div><div>...</div><div><br></div><div>Formatting: You have two 80-col violations, but otherwise looks great.</div><div><br></div><div>Typo:</div><div>+ // ... loop</div><div>+ // loop prologue</div><div><br></div><div>---</div><div>In EmitRuntimePrologueFixup:</div><div><br></div><div>+/// extraiters = tripcount % loopfactor</div><div>+/// if (extraiters == 0) jump Loop:</div><div>+/// if (extraiters == loopfactor) jump L1</div><div>+/// if (extraiters == loopfactor-1) jump L2</div><div><br></div><div>You may want to comment that you expect a later pass to convert this</div><div>to a switch.</div><div><br></div><div>Before you erase PH->terminator, maybe you should assert that it has one</div><div>successor pointing to PEnd.</div><div><br></div><div>+ std::vector<BasicBlock*> NewBlocks(LoopBlocks.size());</div><div><br></div><div>No need to initialize the vector, then immediately clear it. You can use</div><div>NewBlocks.reserve(LoopBlocks.size()).</div><div><br></div><div>+ for (unsigned i = Count-1; i > 0; --i) {</div><div><br></div><div>This is a large, complex loop. To be more self-documenting, use a</div><div>meaningful variable like "leftOverIters". Then it will be clear why</div><div>you're counting down, and the nested loop won't hide the variable.</div><div><br></div><div>+ if (i != Count-1) {</div><div><br></div><div>My personal preference, I'd like to see the i == Count-1 case first</div><div>since that's what happens first.</div><div><br></div><div>---</div><div>In CloneLoopBlocks:</div><div><br></div><div>+ LPM->cloneBasicBlockSimpleAnalysis(LoopBlocks[j], NewBB, L);</div><div><br></div><div>I'm not sure this makes sense given that the new block is in the</div><div>parent loop. It may happen to work for the only user, LICM. Since</div><div>you're not doing anything to enforce safety in the API, I would add</div><div>comments in your code, in the API declaration, and in LICM that 'L' is</div><div>the "From" block's loop, not necessarily the "To" block's loop.</div><div><br></div><div>+ if (InsertTop != 0) {</div><div><br></div><div>Why isn't InsertTop set to PH for the first copy?</div><div><br></div><div>+ // Update our running map of newest clones</div><div>+ for (ValueToValueMapTy::iterator VI = VMap.begin(), VE = VMap.end();</div><div><br></div><div>If possible, avoid copying the map. For example, call LVMap.swap(VMap)</div><div>later, after you're done with the current loop iteration, before</div><div>RemapInstrucion. You might need more explicit handling of header phis</div><div>and latch terminators, but it's probably worth it both in efficiency</div><div>and code clarity. I realize the current unroller does this, but it</div><div>think the copying wasn't removed only because it does membership</div><div>checks on LastValueMap, which you don't need.</div><div><br></div><div>In runtime-loop3.ll please use FileCheck instead of grep. We're trying</div><div>to minimize dependence on external tools in the unit tests.</div><div><br></div><div>-Andy</div></div></body></html>