[PATCH] D21330: Loop vectorization with FP induction variables

[Elena Demikhovsky via llvm-commits]

delena marked 2 inline comments as done.
delena added a comment.

Michael, thanks a lot for your comments. I'll upload a new patch.


================
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()) &&
----------------
delena wrote:
> sanjoy wrote:
> > mkuper wrote:
> > > 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)?
> > Why do we need this?
> I defined Step as unknown SCEV, but underlying value is of floating point type.
> In this case expand() just returns the underlying value, but it fails on one of these two lines.
I don't know why do we need to pass the type. 
May be for casting between i64 and PointerType?
But an Unknown SCEV may have any type, right?

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

================
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");
----------------
mkuper wrote:
> As long as you're touching this - maybe hoist this assert for all 3 cases above the switch?
Ok

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:739
@@ +738,3 @@
+    if (BinaryOperator::isFNeg(StepValue)) {
+      Value *MulExp =
+        B.CreateFMul(cast<BinaryOperator>(StepValue)->getOperand(1), Index);
----------------
mkuper wrote:
> Why do we need this?
I should cover fsub operation somehow.
;  for (int i=0; i < N; ++i) {
;    A[i] = x;
;    x -= fp_inc;
;  }
I keep Step as Fneg(fp_inc) in this case.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:772
@@ +771,3 @@
+        BEValueV = V;
+      else if (BEValueV != V)
+        return false;
----------------
mkuper wrote:
> 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?
I just copied this code from integer Phi together with weird variable names.
Can we have multiple bbs inside loop?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:801
@@ +800,3 @@
+        Addend = BinaryOperator::CreateFNeg(I);
+        cast<Instruction>(Addend)->insertAfter(I);
+      }
----------------
mkuper wrote:
> Are you sure this is always a safe place to insert? E.g. what if Addend is a PHI in an outer loop?
This FNeg disappears after transformation. It is just a way to distinguish between FAdd and FSub.

================
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;
----------------
mkuper wrote:
> Why not isFloatingPointTy() here?
isFloatingPointTy() includes more than half, float and double. I'm not sure I want to cover other types.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2178
@@ +2177,3 @@
+  bool FNegativeStep = false;
+  if (BinaryOperator::isFNeg(Step)) {
+    Step = cast<BinaryOperator>(Step)->getOperand(1);
----------------
mkuper wrote:
> 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?
I don't see any problem. x + (-a) is equal to x - a.


Repository:
  rL LLVM

http://reviews.llvm.org/D21330