[PATCH] D13402: Refactor loop unrolling pass and add optimization remarks

[Mark Heffernan via llvm-commits]

On Fri, Oct 16, 2015 at 12:06 PM, Michael Zolotukhin <mzolotukhin at apple.com>
wrote:

> 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.
>

Sounds good. I'll break it into pieces and send them out.


> 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?
>

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.

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):

unroll.c:
void foo(int *p) {
  #pragma clang loop unroll_count(16)
  for (int i = 0; i < 1024; i++) {
    p[i]++;
  }
}

clang  /tmp/unroll.c -Rpass=loop-unroll -Rpass-analysis=loop-unroll
 -Rpass-missed=loop-unroll -O2  -S -emit-llvm -c -o /tmp/unroll.ll
/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]
  for (int i = 0; i < 1024; i++) {
  ^
/tmp/unroll.c:33:3: remark: unrolled loop by a factor of 16 with a breakout
at trip 0 [-Rpass=loop-unroll]
/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]
/tmp/unroll.c:33:3: remark: loop not unrolled: unrolling is explicitly
disabled [-Rpass-analysis=loop-unroll]

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'.

Also, please find some comments inline.
>

I'll incorporate these into the split changes.

Thanks,
Mark


>
> Thanks,
> Michael
>
>
> ================
> Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:83
> @@ +82,3 @@
> +class LoopDescription;
> +class UnrollLimits;
> +
> ----------------
> `UnrollLimits` is a struct, not a class, right?
>
> ================
> Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:248
> @@ +247,3 @@
> +// decision.
> +class LoopDescription {
> +public:
> ----------------
> 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.
>
>
> http://reviews.llvm.org/D13402
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151016/ca590f45/attachment.html>