[PATCH] D19553: Unroll pass restructure.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Evgeny,

Thanks for doing this, I agree that this place is like a big mess now. Please find some comments inline.

Michael


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

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:528
@@ +527,3 @@
+// Calculates unroll count and writes it to UP.Count.
+static bool getUnrollCount(Loop *L, const TargetTransformInfo &TTI,
+                           DominatorTree &DT, LoopInfo *LI, ScalarEvolution *SE,
----------------
I would rename it to `computeUnrollCount`, `findUnrollCount`, or something like this. It looks to me that we're doing much more work in this function than just 'get'.

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

================
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.");
 
----------------
Can we `assert(UP.Count == TripCount)` here? If yes, it would emphasize the logic, if not, the logic seems flawed.

================
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)
----------------
Is this comment relevant now? And where did the convergence checks go?

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:784
@@ +783,3 @@
+
+  bool isCountSetExplicitly =
+      getUnrollCount(L, TTI, DT, LI, SE, TripCount, TripMultiple, LoopSize, UP);
----------------
s/isCountSetExplicitly/IsCountSetExplicitly/

================
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,
----------------
This change might also be in a separate patch, right?


Repository:
  rL LLVM

http://reviews.llvm.org/D19553