[PATCH] D21330: Loop vectorization with FP induction variables

[Sanjoy Das via llvm-commits]

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

Some comments inline.  Please let me know if you want me to look at something specific, but I'm not familiar enough with the code this patch touches to lgtm it.


================
Comment at: ../lib/Analysis/ScalarEvolutionExpander.cpp:1614
@@ -1613,3 +1613,3 @@
   Value *V = expand(SH);
-  if (Ty) {
+  if (Ty && SE.isSCEVable(Ty)) {
     assert(SE.getTypeSizeInBits(Ty) == SE.getTypeSizeInBits(SH->getType()) &&
----------------
This is odd -- is it just to help keep the `Step` as a `SCEV *`?  If so, I'd suggest solving that within `InductionDescriptor` itself (i.e. maybe support having the step as either a `SCEV *` or a `Value *`, depending on the type of the `InductionDescriptor`?).

================
Comment at: ../lib/Transforms/Utils/LoopUtils.cpp:762
@@ -733,2 +761,3 @@
 
-bool InductionDescriptor::isInductionPHI(PHINode *Phi,
+bool InductionDescriptor::isFpInductionPHI(PHINode *Phi, const Loop *TheLoop,
+                                           ScalarEvolution *SE,
----------------
(Not for fixing in this change) looks like a better interface would be to return an `Optional< InductionDescriptor>`?

================
Comment at: ../lib/Transforms/Utils/LoopUtils.cpp:787
@@ +786,3 @@
+
+  if (!isa<BinaryOperator>(BEValue))
+    return false;
----------------
Maybe use a `dyn_cast` here?

================
Comment at: ../lib/Transforms/Utils/LoopUtils.cpp:811
@@ +810,3 @@
+
+  Instruction *UAI = !BOp->hasUnsafeAlgebra() ? BOp : nullptr;
+  D = InductionDescriptor(StartValue, IK_FpInduction, Step, UAI,
----------------
The condition looks inverted?


Repository:
  rL LLVM

https://reviews.llvm.org/D21330