<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 16, 2015 at 12:06 PM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">In general it looks good, but I'd prefer to split it to smaller patches. That is, separate refactoring (which brings no functionality changes) from adding new stuff (that is, printing remarks). The refactoring, in its turn, should be done in as small steps as possible - e.g. 1) factor out `UnrollParameters` class, 2) factor out `UnrollLimits`, etc.<br></blockquote><div><br></div><div>Sounds good. I'll break it into pieces and send them out.</div><div> <br></div><!--
--><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
One more question: loop-unrolling is currently executed twice in our O3 pipeline, and in theory it's possible that we'll unroll the same loop twice (e.g. partially the first time and completely the second time). In this case we'll emit two remarks for the same loop, which might be confusing for the user. Any thought on how we can avoid doing this?<br></blockquote><div><br></div><div>Even with the O2 pipeline we execute unrolling twice. One option is to emit metadata saying that the loop has been unrolled and by how much. This would not eliminate the double message, but then the second message could be more informative. Something like: "Unrolled loop by a factor of 4; loop had previously been unrolled by a factor of 2 for a total unroll factor of 8". (wording could probably be improved). Not perfect, but it's hard to emit only one definitive message per loop unless the unrolling pass knows it's the last unrolling pass to run which I don't know a <!--
-->nice way of doing.</div><div><br></div><div>Also, this has-been-unrolled metadata could also be used to improve remarks for the '#pragma unroll N' case. In this case we mark the loop with unroll-disable metadata after unrolling by N to avoid additional unrolling by a later unrolling pass. This has the unfortunate effect of a misleading remark for the second unroller pass. Example with the current patch (note the last two remarks):</div><div><br></div><div><div>unroll.c:</div><div>void foo(int *p) {</div><div>  #pragma clang loop unroll_count(16)</div><div>  for (int i = 0; i < 1024; i++) {</div><div>    p[i]++;</div><div>  }</div><div>}</div></div><div><br></div><div><div>clang  /tmp/unroll.c -Rpass=loop-unroll -Rpass-analysis=loop-unroll  -Rpass-missed=loop-unroll -O2  -S -emit-llvm -c -o /tmp/unroll.ll</div><div>/tmp/unroll.c:33:3: remark: candidate loop for unrolling: loop body size = 6, trip count = 1024, loop has pragma suggesting an unroll factor of 16<!--
--> [-Rpass-analysis]</div><div>  for (int i = 0; i < 1024; i++) {</div><div>  ^</div><div>/tmp/unroll.c:33:3: remark: unrolled loop by a factor of 16 with a breakout at trip 0 [-Rpass=loop-unroll]</div><div>/tmp/unroll.c:33:3: remark: candidate loop for unrolling: loop body size = 18, trip count = 64, loop has disable unrolling pragma [-Rpass-analysis=loop-unroll]</div><div>/tmp/unroll.c:33:3: remark: loop not unrolled: unrolling is explicitly disabled [-Rpass-analysis=loop-unroll]</div></div><div><br></div><div>In this case we could avoid emitting these remarks about the loop having a disable unrolling pragma if it has already-unrolled metadata... and now that I think about it this would also eliminate duplicate messaging if the original source code has '#pragma nounroll'.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also, please find some comments inline.<br></blockquote><div><br></div><div>I'll incorporate these into the split changes.</div><div><br></div><div>Thanks,</div><div>Mark</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks,<br>
Michael<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:83<br>
@@ +82,3 @@<br>
+class LoopDescription;<br>
+class UnrollLimits;<br>
+<br>
----------------<br>
`UnrollLimits` is a struct, not a class, right?<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:248<br>
@@ +247,3 @@<br>
+// decision.<br>
+class LoopDescription {<br>
+public:<br>
----------------<br>
I'm a bit worried about this name, as it's very general and doesn't mention unrolling. For instance, currently we have `LoopInfo` which sounds similar, but holds absolutely different information. I'm not saying we want to merge them in any way, but just mentioned it because it might be confusing for the future readers of the code, since `LoopDecscription` has no hint regarding how specialized it is.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D13402" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13402</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>