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

[Ayal Zaks via Phabricator via llvm-commits]

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

LGTM, with some additional thoughts provoked by this fix.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1343
+    if (VF > 1 && Scalars.find(VF) == Scalars.end())
+      return true;
+
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59995