[PATCH] D21330: Loop vectorization with FP induction variables

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Elena.

I guess there's one thing I don't understand about the fnegs - why are you using an IR instruction as the marker of whether the original induction had an fadd or an fsub, instead of a property of the IV? It would make sense to me if you actually used the fneg to feed the vector induction, thus simplifying the code (not having to special-case the sub), but instead you special-case it anyway by looking through the fneg.

Another thing I forgot earlier - this should only fire when we're in unsafe/fast math mode, right? Is there a check for that?


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:739
@@ +738,3 @@
+    if (BinaryOperator::isFNeg(StepValue)) {
+      Value *MulExp =
+        B.CreateFMul(cast<BinaryOperator>(StepValue)->getOperand(1), Index);
----------------
delena wrote:
> 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.
> 
I understand, what I mean is - let's say the step is an FNeg. 
Why can't you feed the FNeg directly into the CreateFMul? Do we get worse code?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:772
@@ +771,3 @@
+        BEValueV = V;
+      else if (BEValueV != V)
+        return false;
----------------
delena wrote:
> 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?
Right now, I think not. (and that needs to be fixed.) :-\
Anyway, if this is a verbatim copy from the int case, leave it be for now. Should probably be fixed separately. 
(Of course, that raises the question - can the int and the fp case share the code? Or is it not worth it?)

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:801
@@ +800,3 @@
+        Addend = BinaryOperator::CreateFNeg(I);
+        cast<Instruction>(Addend)->insertAfter(I);
+      }
----------------
delena wrote:
> 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.
When does it disappear? If it disappears in later clean-up, we still don't want to insert it at an illegal location (like between two phis).
If it's guaranteed to be deleted during the run of the vectorizer, I guess this will technically work, but I'd still prefer to avoid it.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2178
@@ +2177,3 @@
+  bool FNegativeStep = false;
+  if (BinaryOperator::isFNeg(Step)) {
+    Step = cast<BinaryOperator>(Step)->getOperand(1);
----------------
delena wrote:
> 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.
What I'm trying to say is that it's weird that the behavior would be different based on whether the step is an fneg. It could be an fneg because you added an fneg, it could be an fneg because there was already an fneg. Why does this code look through the fneg? It doesn't seem like this should be the vectorizer's job.
Although if the fneg won't get cleaned up later, this is probably the right thing to do.


Repository:
  rL LLVM

http://reviews.llvm.org/D21330