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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5920-5922
+        Optional<unsigned> Idx = getInsertIndex(VU);
+        if (!Idx)
+          continue;
----------------
ABataev wrote:
> 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.
> No, we don't need this check here.
What if `EU.Scalar` is used a second operand?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5920-5922
+        Optional<unsigned> Idx = getInsertIndex(VU);
+        if (!Idx)
+          continue;
----------------
anton-afanasyev wrote:
> ABataev wrote:
> > 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.
> > No, we don't need this check here.
> What if `EU.Scalar` is used a second operand?
Sure, it will optimized in that case, but why do we need this special check for `insertelement` here? Why not for `mul i32 ..., poison`, for instance? If we want to check that the value extracted is used by poison only, we can do this generally in special place.


================
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:
> > > 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.
> > > No, we don't need this check here.
> > What if `EU.Scalar` is used a second operand?
> Sure, it will optimized in that case, but why do we need this special check for `insertelement` here? Why not for `mul i32 ..., poison`, for instance? If we want to check that the value extracted is used by poison only, we can do this generally in special place.
I propose to proceed with current fix and then go on further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120223