[PATCH] D21330: Loop vectorization with FP induction variables

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1610
@@ -1609,3 +1609,3 @@
   Value *V = expand(SH);
-  if (Ty) {
+  if (Ty && SE.isSCEVable(Ty)) {
     assert(SE.getTypeSizeInBits(Ty) == SE.getTypeSizeInBits(SH->getType()) &&
----------------
That sounds a bit weird to me.
It looks like the contract for expandCodeFor is that if you pass a type, you get the expansion cast to this type. What happens if you pass a type, but get a result that's from a different type than you passed (because it's not scevable)?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:675
@@ -674,2 +674,3 @@
          "Step value should be constant for pointer induction");
-  assert(Step->getType()->isIntegerTy() && "StepValue is not an integer");
+  assert(((IK != IK_PtrInduction && IK != IK_IntInduction) ||
+         Step->getType()->isIntegerTy()) && "StepValue is not an integer");
----------------
Maybe ((IK == IK_FpInduction) || Step->getType()->isIntegerTy()) would be clearer.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:678
@@ +677,3 @@
+
+  assert((IK != IK_FpInduction || Step->getType()->isFloatingPointTy()) &&
+         "StepValue is not FP for FpInduction");
----------------
Maybe put this on line 668, next to the two asserts for the other types?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:733
@@ +732,3 @@
+  case IK_FpInduction: {
+    assert(Index->getType() == Step->getType() &&
+           "Index type does not match StepValue type");
----------------
As long as you're touching this - maybe hoist this assert for all 3 cases above the switch?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:739
@@ +738,3 @@
+    if (BinaryOperator::isFNeg(StepValue)) {
+      Value *MulExp =
+        B.CreateFMul(cast<BinaryOperator>(StepValue)->getOperand(1), Index);
----------------
Why do we need this?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:767
@@ +766,3 @@
+  Value *BEValueV = nullptr, *StartValueV = nullptr;
+  for (BasicBlock *Bb : Phi->blocks()) {
+    Value *V = Phi->getIncomingValueForBlock(Bb);
----------------
Some variable naming nitpicking:
Bb -> either BB or bb?
BEValueV -> BEValue
StartValueV -> StartValue

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:772
@@ +771,3 @@
+        BEValueV = V;
+      else if (BEValueV != V)
+        return false;
----------------
Do we really need the if here? We expect L->contains(BB) to be true for one of the incoming values, and false for the other, right? So if we're in the "else", we're already in a bad shape, regardless of whether BEValueV == V or not.
Or did I misunderstand?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:776
@@ +775,3 @@
+      StartValueV = V;
+    else if (StartValueV != V)
+      return false;
----------------
Same as above.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:780
@@ +779,3 @@
+  
+  if (auto *BOp = dyn_cast<BinaryOperator>(BEValueV)) {
+    if (BOp->getOpcode() != Instruction::FAdd &&
----------------
Can we replace this with an early exit?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:801
@@ +800,3 @@
+        Addend = BinaryOperator::CreateFNeg(I);
+        cast<Instruction>(Addend)->insertAfter(I);
+      }
----------------
Are you sure this is always a safe place to insert? E.g. what if Addend is a PHI in an outer loop?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:827
@@ -741,1 +826,3 @@
+  if (!PhiTy->isIntegerTy() && !PhiTy->isPointerTy() &&
+      !PhiTy->isFloatTy() && !PhiTy->isDoubleTy() && !PhiTy->isHalfTy())
     return false;
----------------
Why not isFloatingPointTy() here?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2154
@@ -2152,2 +2153,3 @@
   // Create the types.
   Type *ITy = Val->getType()->getScalarType();
+  int VLen = Val->getType()->getVectorNumElements();
----------------
ITy here stands for "IntegerTy", I guess. Perhaps rename, now that ITy can be float?
(Unless ITy stands for IndexTy, which also make sense, and in which case we should also probably rename. :-) )

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2178
@@ +2177,3 @@
+  bool FNegativeStep = false;
+  if (BinaryOperator::isFNeg(Step)) {
+    Step = cast<BinaryOperator>(Step)->getOperand(1);
----------------
This looks a bit odd.
If I understand correctly, you're relying on the step being FNeg to distinguish whether the original direction of the loop was positive or negative (by transforming a phi that feeds an fsub by a phi that feeds an fadd with an fneg).
What happens if the scalar loop has an fadd, where the original step is a loop-invariant fneg?


Repository:
  rL LLVM

http://reviews.llvm.org/D21330