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

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi,

I've got a bunch of comments - I think it'll be easier to review if you address these first, then I can continue to the end of the patch.

James


================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:436
@@ -429,2 +435,3 @@
       DenseMap<Instruction *, int64_t> &IVToIncMap;
+      Instruction *LoopCtrlOnlyIV;
     };
----------------
I don't see the need for the "Only" here. Simply "LoopControlIV" ? (no need for "ctrl" instead of "control" - be verbose when possible).

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:440
@@ +439,3 @@
+    // Check if it is a compare-like instruction whose user is a branch
+    bool isCompareInst(Instruction *I, BasicBlock *Header) {
+      Instruction *Branch = dyn_cast<BranchInst>(Header->getTerminator());
----------------
The name should include "branch" - like "isCompareUsedByBranch" or something.

It can also be made simpler:

  bool isCompareUsedByBranch(Instruction *I) {
    auto *TI = I->getParent()->getTerminator();
    if (!isa<BranchInst>(TI) || !isa<CmpInst>(I))
      return false;
    return I->hasOneUse() && TI->getOperand(0) == I;
  }

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:528
@@ +527,3 @@
+//    used by PHI, compare is only used by branch.
+bool LoopReroll::isLoopCtrlOnlyIV(Loop *L, Instruction *IV) {
+
----------------
Can't you just use InductionInfo::isInduction() here? You'd be looking for an induction with a step of 1 that is used by the loop latch. It seems this could remove a lot of code.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:530
@@ +529,3 @@
+
+  int32_t IVUses = IV->getNumUses();
+  if (!(IVUses == 2 || IVUses == 1))
----------------
Use "unsigned" here. There's no need to fix this to a specific bitwidth and no need for it to be signed.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:531
@@ +530,3 @@
+  int32_t IVUses = IV->getNumUses();
+  if (!(IVUses == 2 || IVUses == 1))
+    return false;
----------------
Please fold the ! inside the brackets for readability:

  if (IVUses != 2 && IVUses != 1)
    return false;

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:538
@@ +537,3 @@
+    int32_t IncOrCmpUses = User->getNumUses();
+    bool isCompInst = isCompareInst(dyn_cast<Instruction>(User), Header);
+
----------------
IsCompInst

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:538
@@ +537,3 @@
+    int32_t IncOrCmpUses = User->getNumUses();
+    bool isCompInst = isCompareInst(dyn_cast<Instruction>(User), Header);
+
----------------
jmolloy wrote:
> IsCompInst
isCompareInst doesn't handle nullptr. This should either use cast<Instruction>() or check that dyn_cast<Instruction>() is not nullptr.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:541
@@ +540,3 @@
+    // User can only have one or two uses.
+    if (!(IncOrCmpUses == 2 || IncOrCmpUses == 1))
+      return false;
----------------
Same as line 531.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:546
@@ +545,3 @@
+    if (IVUses == 1) {
+      // The only user must be loop increment.
+      if (isCompInst)
----------------
You can fold these two if's together.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:614
@@ +613,3 @@
+
+        if (isLoopCtrlOnlyIV(L, &(*I))) {
+          assert(!LoopCtrlOnlyIV && "Found two loop control only IV");
----------------
No need for brackets: &*I.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1170
@@ +1169,3 @@
+  BasicBlock *Header = L->getHeader();
+  if (LoopCtrlOnlyIV && LoopCtrlOnlyIV != IV) {
+    for (auto *U : LoopCtrlOnlyIV->users()) {
----------------
This is really nasty. Please come up with a more concise way of expressing this.

(If you used InductionDescriptor to represent LoopControlIV, this should be fairly easy I think?)


Repository:
  rL LLVM

http://reviews.llvm.org/D16550