[PATCH] D19553: Unroll pass restructure.

[Evgeny Stupachenko via llvm-commits]

evstupac added a comment.

Hi Michael,

Thanks for looking into proposed new structure.
I'll submit a set of patches for restructure. Finally I want to move all checks preventing unroll from "lib/Transforms/Utils/LoopUnroll.cpp" to "lib/Transforms/Scalar/LoopUnrollPass.cpp".

See my answers inline.

Thanks,
Evgeny


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:83-87
@@ -82,2 +82,7 @@
 static cl::opt<bool>
+UnrollAllowRemainder("unroll-allow-remainder", cl::Hidden,
+  cl::desc("Allow generation of a loop remainder (extra iterations) "
+           "when unrolling a loop."));
+
+static cl::opt<bool>
 UnrollRuntime("unroll-runtime", cl::ZeroOrMore, cl::Hidden,
----------------
mzolotukhin wrote:
> Do we need `unroll-runtime` at all if we have this option? Also, I'd suggest to separate code restructuring from adding new stuff, like new command line options.
This is to structure case with "Convergent" operation in a loop and face needs of architectures that suffers from generated remainder.
However I agree that option itself could be removed from the patch if UP.AllowRemainder remain "true" by default.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:536-538
@@ +535,5 @@
+  if (UnrollCount.getNumOccurrences() > 0) {
+    UP.Count = UnrollCount;
+    UP.AllowExpensiveTripCount = true;
+    UP.Force = true;
+    if ((LoopSize - 2) * UP.Count + 2 < UP.Threshold
----------------
mzolotukhin wrote:
> We set these parameters here, but we're not guaranteed to early exit. Is this intentional? Maybe set them only if we're going to return?
> Also, do we need to set `UP.Runtime = true` as we do below?
> We set these parameters here, but we're not guaranteed to early exit. Is this intentional?
> 

Yes, since user requested unroll. Only exceeded pragma threshold or unsafe transformation can prevent unroll here. When pragma threshold is exceeded we still allow all kinds of unroll, but with less Count.


> Also, do we need to set UP.Runtime = true as we do below?
> 

Yes UP.Runtime whold be reasonable here. However that will cause some tests fail. I'll request this in next patch. Right now I tried to keep current logic to highlight places like this.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:661-665
@@ -678,1 +660,7 @@
+  }
+  if (PragmaFullUnroll)
+      emitOptimizationRemarkMissed(
+          Ctx, DEBUG_TYPE, *F, LoopLoc,
+          "Unable to fully unroll loop as directed by unroll(full) pragma "
+          "because loop has a runtime trip count.");
 
----------------
mzolotukhin wrote:
> Can we `assert(UP.Count == TripCount)` here? If yes, it would emphasize the logic, if not, the logic seems flawed.
assert(TripCount) will be more accurate as UP.Count could be 0

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:696-707
@@ -689,12 +695,14 @@
+  if (!UP.AllowRemainder && UP.Count != 0 && (TripMultiple % UP.Count) != 0) {
     // If the loop contains a convergent operation, the prelude we'd add
     // to do the first few instructions before we hit the unrolled loop
     // is unsafe -- it adds a control-flow dependency to the convergent
     // operation.  Therefore Count must divide TripMultiple.
     //
     // TODO: This is quite conservative.  In practice, convergent_op()
     // is likely to be called unconditionally in the loop.  In this
     // case, the program would be ill-formed (on most architectures)
     // unless n were the same on all threads in a thread group.
     // Assuming n is the same on all threads, any kind of unrolling is
     // safe.  But currently llvm's notion of convergence isn't powerful
     // enough to express this.
+    while (UP.Count != 0 && TripMultiple % UP.Count != 0)
----------------
mzolotukhin wrote:
> Is this comment relevant now? And where did the convergence checks go?
lines 781 - 782. I agree it is better to move the comment (with corresponding changes) there.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:202
@@ -201,3 +201,3 @@
 /// DominatorTree if they are non-null.
-bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount,
+bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force,
                       bool AllowRuntime, bool AllowExpensiveTripCount,
----------------
mzolotukhin wrote:
> This change might also be in a separate patch, right?
Yes. But I'd like to keep it in this patch. That is related to current "-unroll-count" and "#pragma unroll" behavior.

Now:
 "-unroll-count" forces "full unroll", "partial" and skips "runtime", so go directly to forced unroll when TripCount is runtime.
 "#pragma unroll" forces "full unroll", "partial" and "runtime" and skip forced unroll even if runtime has failed

So to make current behavior clear I'd like to keep this. However you are right  this could be done in separate patch.


Repository:
  rL LLVM

http://reviews.llvm.org/D19553