[PATCH] D10477: Expand loop reroll to handle loop with multiple induction variables and negative increment -part 1/3

[Michael Zolotukhin]

mzolotukhin added a subscriber: mzolotukhin.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:169
@@ -168,2 +168,3 @@
 
-    // A chain of isomorphic instructions, indentified by a single-use PHI,
+    // Map between induction variable and it's increment
+    DenseMap<Instruction *, int64_t> IVToIncMap;
----------------
s/it's/its/

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:172
@@ -170,1 +171,3 @@
+
+    // A chain of isomorphic instructions, identified by a single-use PHI,
     // representing a reduction. Only the last value may be used outside the
----------------
I'd rather commit this typo fix immediately when I found it (no pre-commit review is required for this kind of changes).

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:438
@@ -432,1 +437,3 @@
       UsesTy Uses;
+      // Map between induction variable and it's increment
+      DenseMap<Instruction *, int64_t> &IVToIncMap;
----------------
s/it's/its/

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:665
@@ -658,2 +664,3 @@
   BinaryOperator *BO = dyn_cast<BinaryOperator>(U);
+
   if (!BO || BO->getOpcode() != Instruction::Add)
----------------
No need to add a blank line here.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1268
@@ -1269,2 +1267,3 @@
   }
+  bool    Negative = IVToIncMap[IV] < 0;
   const DataLayout &DL = Header->getModule()->getDataLayout();
----------------
Redundant whitespaces.

================
Comment at: test/Transforms/LoopReroll/negative.ll:19-27
@@ +18,11 @@
+;CHECK-NEXT:    %5 = mul i32 %indvar, -1
+;CHECK-NEXT:    %6 = add i32 %len, %5
+;CHECK-NEXT:    %idxprom = sext i32 %6 to i64
+;CHECK-NEXT:    %arrayidx = getelementptr inbounds i8, i8* %buf, i64 %idxprom
+;CHECK-NEXT:    %7 = load i8, i8* %arrayidx, align 1
+;CHECK-NEXT:    %conv = zext i8 %7 to i64
+;CHECK-NEXT:    %add = add i64 %conv, %sum4.015
+;CHECK-NEXT:    %indvar.next = add i32 %indvar, 1
+;CHECK-NEXT:    %exitcond = icmp eq i32 %6, %4
+;CHECK-NEXT:    br i1 %exitcond, label %while.cond.while.end_crit_edge, label %while.body
+
----------------
Do we really need to check that we generate *exactly* these statements? Would it be possible to look for, say, a specific branch (or absence of the branch) - this would make the test much more stable.

================
Comment at: test/Transforms/LoopReroll/negative.ll:29-38
@@ +28,12 @@
+
+  %sum4.015 = phi i64 [ 0, %while.body.lr.ph ], [ %add4, %while.body ]
+  %len.addr.014 = phi i32 [ %len, %while.body.lr.ph ], [ %sub5, %while.body ]
+  %idxprom = sext i32 %len.addr.014 to i64
+  %arrayidx = getelementptr inbounds i8, i8* %buf, i64 %idxprom
+  %0 = load i8, i8* %arrayidx, align 1
+  %conv = zext i8 %0 to i64
+  %add = add i64 %conv, %sum4.015
+  %sub = add nsw i32 %len.addr.014, -1
+  %idxprom1 = sext i32 %sub to i64
+  %arrayidx2 = getelementptr inbounds i8, i8* %buf, i64 %idxprom1
+  %1 = load i8, i8* %arrayidx2, align 1
----------------
Do we really need to have this body, or would the test work with almost empty loop?
Like this:
```
while (len > 1) {
            len -= 2;
        }
```
instead of 
```
while (len > 1) {
            sum4 += buf[len];
            sum4 += buf[len-1];
            len -= 2;
        }
```
Since you only run loop-reroll, it shouldn't be collapsed to nop.

================
Comment at: test/Transforms/LoopReroll/negative.ll:53-54
@@ +52,4 @@
+  %sum4.0.lcssa = phi i32 [ %phitmp, %while.cond.while.end_crit_edge ], [ 0, %entry ]
+  %call = tail call i32 @goo(i32 %sum4.0.lcssa)
+  unreachable
+}
----------------
Maybe just return `i32 %sum4.0.lcssa` to get rid of the `goo` function?


Repository:
  rL LLVM

http://reviews.llvm.org/D10477