[PATCH] D16550: Reroll loops with multiple IV and negative step part 3/3 -- support multiple induction variables

[Hal Finkel via llvm-commits]

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