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

[Evgeny Stupachenko via llvm-commits]

evstupac added inline comments.

================
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
----------------
mzolotukhin wrote:
> You could use range-loop here.
Almost the same loops to iterate PHIs are widely used in Unroll code. If we change it here we should change everywhere.
As for the range itself, do you mean we can construct vector of PHIs and iterate them?

================
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]
+    //
----------------
mzolotukhin wrote:
> Maybe use IR syntax in the comments?
Will make it more closer to IR, however I'd like to keep variable names from code here.

================
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);
+
----------------
mzolotukhin wrote:
> Why is it `UndefValue`?
Since we added a branch around the Loop to NewExit this UndefValue will be updated correctly (the same technique is used in ConnectProlog). Here we can find value from PreHeader based on V from Latch, however it will be more complicated and further update.

================
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))
----------------
mzolotukhin wrote:
> Why not
> ```
> Instruction *I = dyn_cast<Instruction>(PN->getIncomingValueForBlock(Latch));
> ```
> ? And then use `I` instead of `V`.
We'll need an additional cast in this case as VMap[I] returns not an Instruction.
And add additional check to add incoming value to EpilogPN, verifying when I is null (to add UndefValue).

================
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();
----------------
mzolotukhin wrote:
> You could do
> ```
> for (BasicBlock *Succ : successors(Latch))
> ```
Will update.

================
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;
+
----------------
mzolotukhin wrote:
> We can just check `if(L->contains(*SBI))` before the loop, right?
Correct. There is similar place in LoopUnroll.cpp which has this check in place already.


Repository:
  rL LLVM

http://reviews.llvm.org/D18158