[PATCH] D16550: Reroll loops with multiple IV and negative step part 3/3 -- support multiple induction variables
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 18 22:30:41 PDT 2016
hfinkel added inline comments.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:167
@@ +166,3 @@
+ // For loop with multiple induction variable, remember
+ // The one only for loop control
+ Instruction *LoopControlIV;
----------------
// For a loop with multiple induction variables, remember the one used only to control the loop.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:513
@@ -496,1 +512,3 @@
+// Check if an IV is only for loop control purpose:
+// 1. It only has one use which is loop increment, then it is only used
----------------
// Check if an IV is only used to control the loop. There are two cases:
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:514
@@ +513,3 @@
+// Check if an IV is only for loop control purpose:
+// 1. It only has one use which is loop increment, then it is only used
+// by compare and PHI, compare is only used by branch.
----------------
// 1. It has only one use which is the loop increment, and the increment is only used by the comparison and the PHI, and the comparison is used only by the branch.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:516
@@ +515,3 @@
+// by compare and PHI, compare is only used by branch.
+// 2. It is used by loop increment and compare, loop increment is only
+// used by PHI, compare is only used by branch.
----------------
// 2. It is used only by the loop increment and the comparison, the loop increment is only used by the PHI, and the comparison is used only by the branch.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:534
@@ +533,3 @@
+ if (IVUses == 1) {
+ // The only user must be loop increment.
+ // Loop increment must have two uses.
----------------
be the loop increment
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:535
@@ +534,3 @@
+ // The only user must be loop increment.
+ // Loop increment must have two uses.
+ if (IsCompInst || IncOrCmpUses != 2)
----------------
The loop increment
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:544
@@ +543,3 @@
+
+ // All users of IV must be binary operation or cmp
+ if (auto *BO = dyn_cast<BinaryOperator>(User)) {
----------------
// The users of the IV must be a binary operation or a comparison.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1152
@@ -1074,1 +1151,3 @@
+ // Make sure we mark the loop ctrl only PHIs as used in all iterations.
+ // Including loop increment, compares and branches, there are two cases:
----------------
// Make sure we mark loop-control-only PHIs as used in all iterations. See comment above LoopReroll::isLoopControlIV for more information.
[Don't repeat the comment text from above LoopReroll::isLoopControlIV here.]
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1170
@@ +1169,3 @@
+ if (UU->hasOneUse()) {
+ Instruction *BI = dyn_cast<BranchInst>(*(UUser->user_begin()));
+ if (BI == dyn_cast<BranchInst>(Header->getTerminator()))
----------------
You don't need the parenthesis around UUser->user_begin() - '->' has higher precedence than '*'.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1171
@@ +1170,3 @@
+ Instruction *BI = dyn_cast<BranchInst>(*(UUser->user_begin()));
+ if (BI == dyn_cast<BranchInst>(Header->getTerminator()))
+ Uses[BI].set(IL_All);
----------------
Do you need, or intend, the dyn_cast here? I think you don't, because you don't want the condition to be true if both sides are nullptr.
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1435
@@ -1325,1 +1434,3 @@
+// For non loop control IVs, we only need to update the last increment
+// with right amount, then we are done.
----------------
non-loop-control IVs
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1451
@@ +1450,3 @@
+ if (!COp)
+ assert(false && "Didn't find constant operand of LoopInc!\n");
+
----------------
assert(COp && "Didn't find constant operand of LoopInc!");
================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1455
@@ +1454,3 @@
+ const SCEV *ScaleSCEV = SE->getConstant(COp->getType(), Scale);
+ if (AInt.isNegative()) {
+ NewInc = SE->getNegativeSCEV(COp);
----------------
It does not look like there are test cases covering this logic, please add some.
Repository:
rL LLVM
http://reviews.llvm.org/D16550
More information about the llvm-commits
mailing list