[PATCH] D22869: [LV] Generate both scalar and vector integer induction variables

[Michael Kuperstein via llvm-commits]

mkuper accepted this revision.
mkuper added a comment.
This revision is now accepted and ready to land.

LGTM after the Invariant -> Contains change.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1972
@@ -1968,3 +1971,3 @@
   // vectorization, it will remain scalar.
   if (Uniforms.count(I) || !TheLoop->contains(I))
     return true;
----------------
mssimpso wrote:
> mkuper wrote:
> > Oh, so if your only scalar users are outside the loop, then you don't want to generate a scalar IV. Makes sense.
> > So you still end up with an extract from the vector IV, right?
> > 
> > Anyway, if all what you want to check is whether the user is in the loop, why are you checking isLoopInvariant and not Contains (like isScalarAfterVectorization does)?
> > So you still end up with an extract from the vector IV, right?
> I don't think so. If there is a user of the induction variable outside the loop (but no scalar users in the loop), yes, the induction variable would remain vector. The external induction variable users are handled in fixupIVUsers (this was recently added I think). And I think the fixup's are based on the canonical induction variable.
> 
> > Anyway, if all what you want to check is whether the user is in the loop, why are you checking isLoopInvariant and not Contains (like isScalarAfterVectorization does)?
> isLoopInvariant does the additional check that the value is an instruction. But I'm thinking we should just cast the user to an instruction here and use Contains. That should make this more clear. I'll update the patch.
> The external induction variable users are handled in fixupIVUsers (this was recently added I think). And I think the fixup's are based on the canonical induction variable.
Yes, I've added that, I just wanted to make sure this doesn't interfere, and was too lazy to look up what I actually did. :-)
Anyway, you're right, we'll always have the canonical IV as scalar, so it's fine.




https://reviews.llvm.org/D22869