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

[James Molloy via llvm-commits]

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

Hi,

Thanks for working on this. I have plenty of comments :)

Cheers,

James


================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:475
@@ -473,1 +474,3 @@
 
+static const SCEVConstant *getConstantCoefFromStep(ScalarEvolution *SE,
+                                                   const SCEV *SCEVExpr,
----------------
This only works for pointers, not for integer steps, so can this be renamed to make that clear?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:489
@@ +488,3 @@
+          SE->getSizeOfExpr(SE->getEffectiveSCEVType(IV->getType()), ElTy);
+      const SCEV *NewSCEV = nullptr;
+      if (IncSCEV->getValue()->getValue().isNegative()) {
----------------
Why declare NewSCEV here, instead of inside the if?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:510
@@ +509,3 @@
+      if (Unknown->isSizeOf(AllocTy)) {
+        DEBUG(dbgs() << "LRR: SizeOf SCEVExpr: " << *Unknown << "\n");
+      } else
----------------
I think this debug statement might not be that useful - can it be removed when committed?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:511
@@ +510,3 @@
+        DEBUG(dbgs() << "LRR: SizeOf SCEVExpr: " << *Unknown << "\n");
+      } else
+        break;
----------------
Braces {} around the else. if/elses should either both have braces or both not.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:512
@@ +511,3 @@
+      } else
+        break;
+    } else
----------------
Surely these breaks should be "return nullptr"? What if the first operand is a constant but the second is not an unknown? or is an unknown but !isSizeOf()? then currently you'll return the first operand but actually this method should fail.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:528
@@ -482,3 +527,3 @@
       continue;
-    if (!I->getType()->isIntegerTy())
+    if (!(I->getType()->isIntegerTy() || I->getType()->isPointerTy()))
       continue;
----------------
This would read better as:

  if (!I->getType()->isIntegerTy() && !I->getType()->isPointerTy())

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:716
@@ -666,1 +715,3 @@
+
+  if (BO && BO->getOpcode() != Instruction::Add)
     return false;
----------------
It might read better if both these ifs were squashed together?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1360
@@ +1359,3 @@
+
+  const SCEVAddRecExpr *AddRecExpr = nullptr;
+  const SCEV *SizeOfExpr = nullptr;
----------------
I really don't think "AddRecExpr" is any more helpful than "H". How about NewIVSCEV?

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1361
@@ +1360,3 @@
+  const SCEVAddRecExpr *AddRecExpr = nullptr;
+  const SCEV *SizeOfExpr = nullptr;
+  const SCEV *IncrExpr =
----------------
I don't like that these are declared outside of scope and initialized to nullptr - this isn't C, we can declare variables where we use them! :) Please sink these into the if expression (you can just use auto too)

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1364
@@ +1363,3 @@
+      SE->getConstant(RealIVSCEV->getType(), Negative ? -1 : 1);
+  if (Inst->getType()->isPointerTy()) {
+    const PointerType *PTy = dyn_cast<PointerType>(Inst->getType());
----------------
You need to use cast<> instead of dyn_cast<> below. Instead, what you can do is this:

    if (auto *PTy = dyn_cast<PointerTy>(Inst->getType())) {

then just use PTy below.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1371
@@ +1370,3 @@
+  }
+  AddRecExpr = cast<SCEVAddRecExpr>(
+      SE->getAddRecExpr(Start, IncrExpr, L, SCEV::FlagAnyWrap));
----------------
I realise it was like this before, but I don't actually think we need this to be of type SCEVAddRecExpr. I think const SCEV * should be sufficient, which means you can remove this cast.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1373
@@ +1372,3 @@
+      SE->getAddRecExpr(Start, IncrExpr, L, SCEV::FlagAnyWrap));
+  // Limit the lifetime of SCEVExpander.
+  const DataLayout &DL = Header->getModule()->getDataLayout();
----------------
I notice you've removed the compound braces around this but kept the comment... either keep the braces or remove the comment too, but give a rationale for removal if you're going to do that.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1386
@@ +1385,3 @@
+    if (Uses[BI].find_first() == IL_All) {
+      const SCEV *ICSCEV = nullptr;
+
----------------
This nullptr initialization again... can we get rid of these? I mean, there seems to be no reason to even touch this line :/

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1394
@@ +1393,3 @@
+      if (Inst->getType()->isPointerTy())
+        Minus1SCEV = SE->getMulExpr(Minus1SCEV, SizeOfExpr);
+
----------------
This is disingenuous, as Minus1SCEV no longer holds minus one! The variable should be renamed.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1401
@@ +1400,3 @@
+        ICMinus1 = Expander.expandCodeFor(ICMinus1SCEV, NewIV->getType(), BI);
+      else {
+        BasicBlock *Preheader = L->getLoopPreheader();
----------------
Please replace the braces you've removed here. Mixing one-liners and braces isn't LLVM style.


Repository:
  rL LLVM

http://reviews.llvm.org/D13151