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

[James Molloy]

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

Hi,

Thanks for making these changes - it looks much better now. A few comments still.

Cheers,

James


================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:496
@@ +495,3 @@
+            continue;
+            IVToIncMap[I] = IncSCEV->getValue()->getZExtValue();
+        }
----------------
I think this line and line 503 can both be merged, and put on line 505. There's no need to ZExt this one and SExt the other - both can be SExted.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:499
@@ +498,3 @@
+        else if (AInt.isNegative()) {
+          const SCEVConstant *PIncSCEV = 
+            dyn_cast<SCEVConstant>(SE->getNegativeSCEV(IncSCEV));
----------------
This can be much simpler. You can just use APInt::abs():

    AInt = AInt.abs();
    if (AInt.isZero() || AInt.uge(MaxInc))
      continue;
    IVToIncMap[I] = AInt.getSExtValue();



================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:677
@@ -661,2 +676,3 @@
 
-  for (auto *UU : BO->users()) {
+  if (!BO && !isa<GetElementPtrInst>(U))
+    return false;
----------------
I'm not sure how adding GEP support here relates to negative values. Was this an accidentally added hunk?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:725
@@ -706,9 +724,3 @@
 
-    // FIXME: Add support for negative values.
-    if (V < 0) {
-      DEBUG(dbgs() << "LRR: Aborting due to negative value: " << V << "\n");
-      return false;
-    }
-
-    Roots[V] = cast<Instruction>(I);
+    Roots[std::abs(V)] = cast<Instruction>(I);
   }
----------------
I think you need to use std::abs() above, on line 721 too. Otherwise the guard on line 721 is useless.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1280
@@ -1270,1 +1279,3 @@
+  int64_t Inc      = IVToIncMap[IV];
+  bool    Negative = Inc < 0;;
   const DataLayout &DL = Header->getModule()->getDataLayout();
----------------
Double semicolon (;;).

Also, you can squash this into just one statement:

    bool Negative = IVToIncMap[IV] < 0;


Repository:
  rL LLVM

http://reviews.llvm.org/D10477