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

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5920-5922
+        Optional<unsigned> Idx = getInsertIndex(VU);
+        if (!Idx)
+          continue;
----------------
ABataev wrote:
> bcahoon wrote:
> > ABataev wrote:
> > > bcahoon wrote:
> > > > ABataev wrote:
> > > > > ABataev wrote:
> > > > > > anton-afanasyev wrote:
> > > > > > > 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?
> > > > > > It means that the scalar is an insertelement instruction, i.e. has vector type, right? It is checked already in line 5908.
> > > > > For the 14.0 branch? I would suggest following more conservative solution, probably:
> > > > > ```
> > > > > Optional<unsigned> Idx = getInsertIndex(VU);
> > > > > if (Idx) {
> > > > >   // Process as shuffle.
> > > > >   ...
> > > > > }
> > > > > ```
> > > > > i.e. do not ignore it but instead count as extractelement cost
> > > > Thanks for the reviews so far. I'm trying to figure out the next steps to fix the assert due to dereferencing the result of getInsertIndex(VU) when it returns None.  What's involved with the comment, 
> > > >   // Process as shuffle.
> > > > Is that the existing code that appears in lines 5923-5949?
> > > > 
> > > > 
> > > Yes
> > Isn't that the behavior of the patch? The existing code is skipped if (!Idx), otherwise Idx contains a valid value, so execute the existing code.  Does something else need to be added?
> No, the patch does `continue`, i.e. skips everything, but instead it should fallback to the cost for extractelement.
Ah, I think I understand. When getInsertValue returns None, the lines 5962-5970 should still occurs rather than being skipped.  Those lines are performing the "cost for extractelement"? I will try that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120223