[PATCH] D120223: [SLP] Fix assert from non-constant index in insertelement

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5920-5922
+        Optional<unsigned> Idx = getInsertIndex(VU);
+        if (!Idx)
+          continue;
----------------
anton-afanasyev wrote:
> anton-afanasyev wrote:
> > ABataev wrote:
> > > I think we need an extra check here. If the index is undefined - continue, but if the index is not constant, need to calculate the cost as extractelement+insertelement.
> > This is just a reversion to previous state, we can proceed with this and commit a separate patch for correct check. There is also another check we need here: that `EU.Scalar` is used as a first operand of insertelement.
> As for `undef` index check, I'm not sure it should be processed specially: we still generate extractelement for `EU.Scalar` at `vectorizeTree()` stage.
No, we don't need this check here.
I was not quite correct in my previous comment. No need to to calculate the cost as extractelement+insertelement, just extractelement.
I mean, we need to have something like this:
```
        Optional<unsigned> Idx = getInsertIndex(VU);
        if (Idx) {
          // Process as shuffle.
          ...
        } else if (isa<Constant>(VY->getOperand(2))
          continue;
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5920-5922
+        Optional<unsigned> Idx = getInsertIndex(VU);
+        if (!Idx)
+          continue;
----------------
ABataev wrote:
> anton-afanasyev wrote:
> > anton-afanasyev wrote:
> > > ABataev wrote:
> > > > I think we need an extra check here. If the index is undefined - continue, but if the index is not constant, need to calculate the cost as extractelement+insertelement.
> > > This is just a reversion to previous state, we can proceed with this and commit a separate patch for correct check. There is also another check we need here: that `EU.Scalar` is used as a first operand of insertelement.
> > As for `undef` index check, I'm not sure it should be processed specially: we still generate extractelement for `EU.Scalar` at `vectorizeTree()` stage.
> No, we don't need this check here.
> I was not quite correct in my previous comment. No need to to calculate the cost as extractelement+insertelement, just extractelement.
> I mean, we need to have something like this:
> ```
>         Optional<unsigned> Idx = getInsertIndex(VU);
>         if (Idx) {
>           // Process as shuffle.
>           ...
>         } else if (isa<Constant>(VY->getOperand(2))
>           continue;
> ```
Yes, but it will optimized out by InstCombiner, since the user with undef index is just an undef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120223