[PATCH] D59995: [LV] Exclude loop-invariant inputs from scalar cost computation.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1343
+    if (VF > 1 && Scalars.find(VF) == Scalars.end())
+      return true;
+
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > Ahh, is it better to assume we can vectorize V, contrary to the above comment?
> > > > Ahh, is it better to assume we can vectorize V, contrary to the above comment?
> > > 
> > > Ah sorry for the change, I missed adding an explanation here. Assuming vectorization here seems to be in line with the old behavior and switching it causes a bunch of unit-test failures. I agree it would be safer to assume scalarizing, but the current cost model seems to expect the optimistic choice here.
> > > 
> > > I might be worth investigating flipping it as a follow-up, but I think it will require some additional work to make sure other places in the cost model are in sync with the new assumption? 
> > OK, fair enough. The current optimistic choice is ok, as long as V is of vectorizable type; and the latter seems to be the case given that setCostBasedWidenDecision() deals with loads and stores only, and LVLegality checks that their relevant operands (stored values) are of vectorizable types. Worth augmenting the explanation if agreed.
> > 
> > In any case, the optimistic choice can be confined to guard isScalarAfterVectorization() only, e.g.,:
> > ```
> >   if (VF <= 1)
> >     return false;
> >   Instruction *I = dyn_cast<Instruction>(V);
> >   if (!I || !TheLoop->contains(I) || TheLoop->isLoopInvariant(I))
> >     return false;
> >   return (Scalars.find(VF) == Scalars.end() || !isScalarAfterVectorization(I, VF));
> > 
> > ```
> > 
> > On a related note, notice that if `%sv` were a <2 x i64> vector instead of a {i64, i64} struct, LVLegality would bail out given that we can't vectorize its associated `extractelement`:
> > ```
> >       // Also, we can't vectorize extractelement instructions.
> >       if ((!VectorType::isValidElementType(I.getType()) &&
> >            !I.getType()->isVoidTy()) ||
> >           isa<ExtractElementInst>(I)) {
> >         reportVectorizationFailure("Found unvectorizable type",
> >             "instruction return type cannot be vectorized",
> >             "CantVectorizeInstructionReturnType", &I);
> >         return false;
> >       }
> > ```
> > but it does allow vectorizing (loops with scalarizing) `extractvalue`. So a crude way of "fixing" the testcase would be to treat extractvalue similar to extractelement... but its clearly better to allow vectorizing such loops. OTOH, perhaps we could also allow vectorizing (loops with scalarizing) `extractelement`...? (Surely in a separate patch if so)
> > OK, fair enough. The current optimistic choice is ok, as long as V is of vectorizable type; and the latter seems to be the case given that setCostBasedWidenDecision() deals with loads and stores only, and LVLegality checks that their relevant operands (stored values) are of vectorizable types. Worth augmenting the explanation if agreed.
> > 
> > In any case, the optimistic choice can be confined to guard isScalarAfterVectorization() 
> 
> Thanks, I'll commit the change with the moved optimistic check as suggested and add a comment.
> 
> > On a related note, notice that if %sv were a <2 x i64> vector instead of a {i64, i64} struct, LVLegality would bail out given that we can't vectorize its associated extractelement:
> 
> I'll look into that as a follow up, same as marking the invariant instructions as uniform.
> OK, fair enough. The current optimistic choice is ok, as long as V is of vectorizable type; and the latter seems to be the case given that setCostBasedWidenDecision() deals with loads and stores only, and LVLegality checks that their relevant operands (stored values) are of vectorizable types. Worth augmenting the explanation if agreed.

I think vectorization should be what is happening in most of those cases, but I think there could be scenarios where we would not vectorize the operands. I've adjusted the comment accordingly.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59995/new/

https://reviews.llvm.org/D59995