[PATCH] D18202: Enable non-power-of-2 pragma unroll counts

[Michael Zolotukhin via llvm-commits]

mzolotukhin accepted this revision.
mzolotukhin added a comment.
This revision is now accepted and ready to land.

Hi,

This patch LGTM, thanks for working on this!

Michael


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:736-739
@@ -740,2 +735,6 @@
 
+  // If loop has an unroll count pragma mark loop as unrolled to prevent
+  // unrolling beyond that requested by the pragma.
+  if (HasPragma && PragmaCount != 0)
+    SetLoopAlreadyUnrolled(L);
   return true;
----------------
But maybe it's not a bad idea to have a 'nounroll' attribute on a loop that we cannot unroll:) Otherwise, next time we'll just probably spend some time trying again.
But I can see arguments for additional attempts to unroll when we have pragma, so if you feel strong about it, please keep it.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:354-355
@@ +353,4 @@
+    // OR
+    //  2. The addition computing TripCount overflowed.
+    //
+    // If (2) is true, we know that TripCount really is (1 << BEWidth) and so
----------------
Oh, makes sense. Phabricator doesn't show whitespace changes, so I missed the change in indentation.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:365-367
@@ +364,5 @@
+                                    ConstantInt::get(BECount->getType(),
+                                                     Count));
+    Value *ModValAdd = B.CreateAdd(ModValTmp,
+                                   ConstantInt::get(ModValTmp->getType(), 1));
+    // At that point (BECount % Count) + 1 could be equal to Count.
----------------
evstupac wrote:
> Yes. That is what I mean by select. I would prefer combiner to decide the correct replacement (maybe based on some architecture properties). The simplification of TripCount % Count to TripCount & (Count - 1) when Count is power-of-2 is pretty obvious and architecture independent. This one is more complicated: ((BECount % Count)  + 1) % Count.
> So I'd let combiner do this if someone find a performance opportunity. 
Thanks for the explanation, it's clear now. I think both `BECount == -1 ? <constant expr> : ((BECount +nuw 1) % Count` and `((BECount % Count) + 1) % Count` are good options here.


http://reviews.llvm.org/D18202