[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:
> > ABataev wrote:
> > > anton-afanasyev wrote:
> > > > 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.
> > > Still need to extract it and count the cost of extract.
> > Generally speaking, it would be good to have such checks, we just can't check everything because of the compile time and amount of work and rely on InstCombiner here, that it transforms such instructions to undefs before SLP pass. We just have some artificial tests, which has undef indices, this check is mostly for such test cases.
> We already underestimate the cost in many cases, would be good to have the proper fix.
Sure. So this is another, main branch. So we need something like `EU.Scalar == VU.getOperand(0)` check here.


================
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:
> > > ABataev wrote:
> > > > anton-afanasyev wrote:
> > > > > 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.
> > > > Still need to extract it and count the cost of extract.
> > > Generally speaking, it would be good to have such checks, we just can't check everything because of the compile time and amount of work and rely on InstCombiner here, that it transforms such instructions to undefs before SLP pass. We just have some artificial tests, which has undef indices, this check is mostly for such test cases.
> > We already underestimate the cost in many cases, would be good to have the proper fix.
> Sure. So this is another, main branch. So we need something like `EU.Scalar == VU.getOperand(0)` check here.
Ok, I see. So do you believe @bcahoon could commit this and then I add other checks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120223