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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Evgeny,

Some comments inline. I hope to take a more detailed look into the actual logic of the patch soon.

Thanks,
Michael


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:173-174
@@ +172,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
----------------
No, I mean
```
for (Instruction &I : NewExit) {
  auto *PN = dyn_cast<PHINode>(&I);
  if (!PN)
    break;
  ...
```
Personally, I find this easier to read than the version with explicit iterators. No need to change the other code, at least definitely not in this patch.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:178-182
@@ +177,7 @@
+    // Basicaly it should look like:
+    // NewExit:
+    //   PN = PHI [I, Latch]
+    // ...
+    // Exit:
+    //   EpilogPN = PHI [PN, EpilogPreHeader]
+    //
----------------
Sure, no complains about the variable names:)

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:193-194
@@ +192,4 @@
+
+    Value *V = PN->getIncomingValueForBlock(Latch);
+    Instruction *I = dyn_cast<Instruction>(V);
+    if (I && L->contains(I))
----------------
Right, I see now, thanks.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:257
@@ -141,2 +256,3 @@
 ///
-static void CloneLoopBlocks(Loop *L, Value *NewIter, const bool UnrollProlog,
+static void CloneLoopBlocks(Loop *L, Value *NewIter, const bool CreateLoop,
+                            const bool UseEpilogRemainder,
----------------
`CreateLoop` is a bit misleading name. Maybe `CreateRemainderLoop` or `CreateLoopForRemainder`? Also, some comments about `InsertTop` and `InsertBot` would be helpful here (I know we didn't have them before, but since you've already dug into the code, it's probably easy for you).

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:543-544
@@ -347,3 +542,4 @@
                                             PreHeaderBR);
-  Value *BECount = Expander.expandCodeFor(BECountSC, BECountSC->getType(),
+  Value *BECount = UseEpilogRemainder ? nullptr :
+                   Expander.expandCodeFor(BECountSC, BECountSC->getType(),
                                           PreHeaderBR);
----------------
Maybe expand it just before the actual use?


Repository:
  rL LLVM

http://reviews.llvm.org/D18158