[PATCH] D52327: [Loop Vectorizer] Abandon vectorization when no integer IV found

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

In https://reviews.llvm.org/D52327#1242351, @hsaito wrote:

> Sorry, I totally forgot about my old patch  https://reviews.llvm.org/D47216.
>
> 1. I like your two different message version better than changing Line 788 condition to if (!WidestIndTy). That's good.
> 2. Please add non-NULL assertion after "Type *IdxTy = Legal->getWidestInductionType();" in InnerLoopVectorizer::getOrCreateTripCount().
> 3. Please include LIT test from https://reviews.llvm.org/D47216.


Thanks Hideki! I'll add the non-NULL assertion for `idxTy`.  Regarding the LIT test from https://reviews.llvm.org/D47216, isn't that essentially the same as the LIT test I have here?

In https://reviews.llvm.org/D52327#1242411, @efriedma wrote:

> Why do we need an integer induction variable?  If one doesn't exist, it should be straightforward to create one.


Makes sense to me, but I'm not experienced in working on the vectorizer, so I'm hesitant to jump into that.  And I'd view it as a separate patch to add that capability.  As Hideki said in https://reviews.llvm.org/D47216:

>> Whether it's legal to convert FP primary induction to INT primary induction and if so under what conditions are debatable, but bailing
>>  out when it's not proven safe (and currently never proven to be safe as far as LV's existing code is concerned) is a valid thing to do.

I don't know how difficult it is to extend it this way, but given that there are explicit expectations that the primary induction is of an integer type, such as:

  assert((IV->getType()->isIntegerTy() || IV != OldInduction) &&
         "Primary induction variable must have an integer type");

it doesn't feel like it's a small tweak.  (But again, I'm not experienced in this area.)
In short, tackling that seems like a fine idea, but a separate task.


https://reviews.llvm.org/D52327