[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 Lawrence,

I've got some more style complaints, but nothing major.

Cheers,

James


================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:385
@@ -384,2 +384,3 @@
       void replace(const SCEV *IterCount);
+      void replaceIV(Instruction *Inst, Instruction *IV, const SCEV *IterCount);
 
----------------
Please either document this or move it to the protected: section below where all the helpers are.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:477
@@ +476,3 @@
+getPointerIncSCEV(ScalarEvolution *SE, const SCEV *SCEVExpr, Instruction *IV) {
+  const SCEVConstant *CIncSCEV = nullptr;
+  const SCEVMulExpr *MulSCEV = dyn_cast<SCEVMulExpr>(SCEVExpr);
----------------
I don't think it's very useful to have this CIncSCEV variable. It'd be a lot neater (and more up to coding style) if instead of "CIncSCEV = ..; ...; return CIncSCEV;", just "return CIncSCEV;"

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:491
@@ +490,3 @@
+            SE->getUDivExpr(SE->getNegativeSCEV(SCEVExpr), SizeOfExpr);
+        CIncSCEV = dyn_cast<SCEVConstant>(SE->getNegativeSCEV(NewSCEV));
+      } else
----------------
   return dyn_cast<SCEVConstant>(...);

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:492
@@ +491,3 @@
+        CIncSCEV = dyn_cast<SCEVConstant>(SE->getNegativeSCEV(NewSCEV));
+      } else
+        CIncSCEV =
----------------
There should either be braces around both the if and else or around neither of them.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:493
@@ +492,3 @@
+      } else
+        CIncSCEV =
+            dyn_cast<SCEVConstant>(SE->getUDivExpr(SCEVExpr, SizeOfExpr));
----------------
return dyn_cast<SCEVConstant>(...)

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:504
@@ +503,3 @@
+  for (const SCEV *Operand : MulSCEV->operands()) {
+    if (const SCEVConstant *Constant = dyn_cast<SCEVConstant>(Operand))
+      CIncSCEV = Constant;
----------------
Braces or no braces, not mixed braces.

================
Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:1386
@@ +1385,3 @@
+        if (Inst->getType()->isPointerTy())
+          MinusPlus1SCEV = SE->getMulExpr(MinusPlus1SCEV, SizeOfExpr);
+
----------------
Please assert(SizeOfExpr); here, to ensure it's initialized (if someone changes the if() above!)


Repository:
  rL LLVM

http://reviews.llvm.org/D13151