[PATCH] D18158: Adding ability to unroll loops using epilogue remainder.
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 17 18:14:07 PDT 2016
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
More information about the llvm-commits
mailing list