[PATCH] D18158: Adding ability to unroll loops using epilogue remainder.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Evgeny,

A bunch of mostly stylistic comments from me are inline, I'd take a closer look later. I'm fine with keeping it in one patch too, usually it's just more convenient to commit small stuff before to move it out of the way.

Michael


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:130
@@ -128,3 +129,3 @@
   // Split the exit to maintain loop canonicalization guarantees
-  SmallVector<BasicBlock*, 4> Preds(pred_begin(Exit), pred_end(Exit));
+  SmallVector<BasicBlock*, 4> Preds(predecessors(Exit));
   SplitBlockPredecessors(Exit, Preds, ".unr-lcssa", DT, LI,
----------------
I was talking mostly about stuff like this. This is very local change, that doesn't depend on the rest of the patch in any way.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:174-175
@@ +173,4 @@
+  // Update PHI nodes at NewExit and Exit.
+  for (BasicBlock::iterator BBI = NewExit->begin();
+       PHINode *PN = dyn_cast<PHINode>(BBI); ++BBI) {
+    // PN should be used in another PHI located in Exit block as
----------------
You could use range-loop here.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:179-183
@@ +178,7 @@
+    // Basicaly it should look like:
+    // NewExit:
+    //   R.NewExit = PN [V, Latch]
+    // ...
+    // Exit:
+    //   R.Exit = EpilogPN [R.NewExit, EpilogPreHeader]
+    //
----------------
Maybe use IR syntax in the comments?

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:192
@@ +191,3 @@
+    // Add incoming PreHeader from branch around the Loop
+    PN->addIncoming(UndefValue::get(PN->getType()), PreHeader);
+
----------------
Why is it `UndefValue`?

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:194-195
@@ +193,4 @@
+
+    Value *V = PN->getIncomingValueForBlock(Latch);
+    Instruction *I = dyn_cast<Instruction>(V);
+    if (I && L->contains(I))
----------------
Why not
```
Instruction *I = dyn_cast<Instruction>(PN->getIncomingValueForBlock(Latch));
```
? And then use `I` instead of `V`.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:218-219
@@ +217,4 @@
+  // Update corresponding PHI nodes in epilog loop.
+  for (succ_iterator SBI = succ_begin(Latch), SBE = succ_end(Latch);
+       SBI != SBE; ++SBI) {
+    for (BasicBlock::iterator BBI = (*SBI)->begin();
----------------
You could do
```
for (BasicBlock *Succ : successors(Latch))
```

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:223-224
@@ +222,4 @@
+      // Skip this as we already updated phis in exit blocks.
+      if (!L->contains(PN))
+        continue;
+
----------------
We can just check `if(L->contains(*SBI))` before the loop, right?

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:235-236
@@ +234,4 @@
+
+      // Update the existing PHI node operand with the value from the
+      // new PHI node.
+      // Corresponding instruction in epilog loop should be PHI.
----------------
Nitpick: this looks wrapped before 80 characters.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:292
@@ -291,3 +440,2 @@
     return false;
-
   // Use Scalar Evolution to compute the trip count. This allows more loops to
----------------
Unnecessary change.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:365-366
@@ -236,3 +364,4 @@
           const MDString *S = dyn_cast<MDString>(MD->getOperand(0));
-          IsUnrollMetadata = S && S->getString().startswith("llvm.loop.unroll.");
+          IsUnrollMetadata =
+              S && S->getString().startswith("llvm.loop.unroll.");
         }
----------------
Unnecessary change.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:446-447
@@ -297,4 +445,4 @@
 
-  // Only unroll loops with a computable trip count, and the trip count needs
-  // to be an int value (allowing a pointer type is a TODO item).
+  // Only unroll loops with a computable trip count and the trip count needs
+  // to be an int value (allowing a pointer type is a TODO item)
   const SCEV *BECountSC = SE->getBackedgeTakenCount(L);
----------------
Nitpick: why remove the dot?

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:480-481
@@ -331,4 +479,4 @@
 
-  // If this loop is nested, then the loop unroller changes the code in the
-  // parent loop, so the Scalar Evolution pass needs to be run again.
+  // If this loop is nested, then the loop unroller changes the code in
+  // parent loop, so the Scalar Evolution pass needs to be run again
   if (Loop *ParentLoop = L->getParentLoop())
----------------
Looks like an unnecessary change.

================
Comment at: test/Transforms/LoopUnroll/AArch64/runtime-loop.ll:1-2
@@ -1,2 +1,3 @@
-; RUN: opt < %s -S -loop-unroll -mtriple aarch64 -mcpu=cortex-a57 | FileCheck %s
+; RUN: opt < %s -S -loop-unroll -unroll-runtime -unroll-count=2 | FileCheck %s -check-prefix=EPILOG
+; RUN: opt < %s -S -loop-unroll -unroll-runtime -unroll-count=2 -unroll-runtime-epilog=false | FileCheck %s -check-prefix=PROLOG
 
----------------
Where did `-mtriple aarch64 -mcpu=cortex-a57` go?

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:37
@@ -36,3 +36,3 @@
 ; CHECK-NOT: udiv
-; CHECK-LABEL: for.body.prol
+; CHECK-LABEL: for.body.epil
 entry:
----------------
These `CHECK` lines were intended to check the code before the loop, so if we start doing remainder iterations in epilogue, then we should look for `for.body` rather than `for.body.epil`, as it will appear first. This doesn't matter much in the current testcase since there are no `udiv` in the loop, but still.


Repository:
  rL LLVM

http://reviews.llvm.org/D18158