<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 26, 2016 at 11:00 AM, Michael Kuperstein <span dir="ltr"><<a href="mailto:mkuper@google.com" target="_blank">mkuper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mkuper added a comment.<br>
<br>
Hi Gerolf,<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D25963#579918" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25963#579918</a>, @Gerolf wrote:<br>
<br>
> Hi,<br>
><br>
> Could you provide more background on this idea?<br>
<br>
<br>
</span>I can't take credit for the idea - this is something GCC already does.<br>
<span class=""><br>
> What is your motivational use case? When the trip count is low why optimize?<br>
<br>
</span>The motivational use case is a loop with a low trip count that is nested inside a loop with a high trip count.<br>
Peeling the inner loop allows further passes in the optimization pipeline simplify the code for the iterations that actually run, making the outer loop's body better.<br>
Consider something like this:<br>
<br>
for (int i = 0; i < bignum; ++i) {<br>
int ret = 0;<br>
for (int j = 0; j < param; ++j)<br>
ret += arr[i][j];<br>
out[i] = ret;<br>
}<br>
<br>
Imagine param is usually 1.<br>
We can then peel this into:<br>
<br>
for (int i = 0; i < bignum; ++i) {<br>
int ret = 0;<br>
if (param == 0)<br>
continue;<br>
ret += arr[i][0]<br>
for (int j = 1; j < param; ++j)<br>
ret += arr[i][j];<br>
out[i] = ret;<br>
}<br>
<br>
Which then becomes something morally equivalent to:<br>
<br>
for (int i = 0; i < bignum; ++i) {<br>
if (param == 0)<br>
continue;<br>
if (param == 1) {<br>
out[i] = arr[i][0];<br>
continue;<br>
}<br>
ret = arr[i][0];<br>
for (int j = 1; j < param; ++j)<br>
ret += arr[i][j];<br>
out[i] = ret;<br>
}<br>
<br>
So, we've improved the common case (param == 1) - we no longer have to initialize ret, we don't have to deal with the inner loop's IV, there's no add, just a direct assignment.<br></blockquote><div><br></div><div>Beyond that, loop unswitching should also kick in which further improves.</div><div><br></div><div><br></div><div>Another example of loop peeling helping eliminating initialization code:</div><div><br></div><div>int res[4] = { 0, 0, 0, 0};</div><div><br></div><div> for (....) {</div><div> res[0] += ... ;</div><div> res[1] += ... ;</div><div> ...</div><div> }</div><div><br></div><div>Peeling one iteration off allows res[] to be directly initialized with first iteration's value.</div><div><br></div><div>Other benefits come from </div><div><br></div><div>1) more cross iteration optimizations cross peeled iterations</div><div>2) better scheduling freedom </div><div>3) possibly improved branch prediction</div><div>4) other special cases where first couple of iterations of the loop do special things (allowing remaining loop body to be cleaner).</div><div><br></div><div><br></div><div>David</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> If the profile is wrong and it actually is a hot loop for a regular/different input set peeling could hurt.<br>
<br>
</span>Sure, but this is true for any profile-guided optimization. PGO is only good with representative inputs. If an optimization was good regardless of input, we'd be doing it for non-PGO builds.<br>
<span class=""><br>
> There are also side effects on code size, register pressure etc. that could hurt performance.<br>
<br>
</span>Right. But that's not really different from any other kind of loop unrolling. Hence the thresholds, etc.<br>
<br>
> Thanks<br>
> Gerolf<br>
<br>
Thanks,<br>
Michael<br>
<span class=""><br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:87<br>
+ VMap[*BB] = NewBB;<br>
+ if (Header == *BB)<br>
+ InsertTop->getTerminator()-><wbr>setSuccessor(0, NewBB);<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Can this be moved outside the loop?<br>
><br>
> assert(VMap[Header]);<br>
> InsertTop->getTerminator()-><wbr>setSuccessor(0, VMap[Header]);<br>
</span>Right, both this and the Latch handling should be moved outside the loop, thanks.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:90<br>
+<br>
+ if (Latch == *BB) {<br>
+ VMap.erase((*BB)-><wbr>getTerminator());<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> This can be handled outside the loop too.<br>
</span>Right, thanks.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:91<br>
+ if (Latch == *BB) {<br>
+ VMap.erase((*BB)-><wbr>getTerminator());<br>
+ BranchInst *LatchBR = cast<BranchInst>(NewBB-><wbr>getTerminator());<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> What this this erase do?<br>
</span>Nothing, nice catch!<br>
(It's stale - it's needed when you replace LatchBR instead of modifying it in-place.)<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:95<br>
+ // If this is the last iteration, branch past the loop, otherwise<br>
+ // branch to the next iteration (which may itself either be peeled off,<br>
+ // or the loop's preheader)<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Is this a stale comment?<br>
</span>No, but I guess it's not clear.<br>
<br>
Let's say we're peeling off K iterations.<br>
<br>
For iteration J in 1..K-1, we want the branch that terminates the copy of the latch to be:<br>
if (cond) goto header(J+1) else goto exit<br>
<br>
For iteration K, we want to set this branch to be:<br>
if (cond) goto new-ph else goto exit.<br>
<br>
Here, new-ph is the preheader of the new loop (that is, the loop that handles iterations >= K+1). Technically, this preheader should be empty, and only contains a branch to the loop header - the only reason it exists is to keep the loop in canonical form.<br>
Does this make sense? If it does, I'll try to improve the comment.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:101<br>
+ // We no longer know anything about the branch probability.<br>
+ LatchBR->setMetadata(<wbr>LLVMContext::MD_prof, nullptr);<br>
+ }<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Why? I think we should update the branch probability here -- it depends on the what iteration of the peeled clone. If peel count < average/estimated trip count, then each peeled iteration should be more biased towards fall through. If peel_count == est trip_count, then the last peel iteration should be biased toward exit.<br>
</span>You're right, it's not that we don't know anything - but we don't know enough. I'm not sure how to attach a reasonable number to this, without knowing the distribution.<br>
Do you have any suggestions? The trivial option would be to assume an extremely narrow distribution (the loop always exits after exactly K iterations), but that would mean having an extreme bias for all of the branches, and I'm not sure that's wise.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>LoopUnrollPeel.cpp:220<br>
+ }<br>
+<br>
+ // Now adjust the phi nodes in the loop header to get their initial values<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> The profile meta data of the original loop's back branch should be adjusted too.<br>
</span>Right, I missed that, thanks.<br>
But, as above - I'm not sure by how much to adjust it.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D25963" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25963</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>