[PATCH] D127073: [SLP] Treat undef as any other constant

Sun Jun 5 09:53:17 PDT 2022

hvdijk added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7598
       if (isConstant(V)) {
         ReuseShuffleIndicies.emplace_back(UniqueValues.size());
         UniqueValues.emplace_back(V);
----------------
ABataev wrote:
> hvdijk wrote:
> > ABataev wrote:
> > > hvdijk wrote:
> > > > ABataev wrote:
> > > > > hvdijk wrote:
> > > > > > ABataev wrote:
> > > > > > > hvdijk wrote:
> > > > > > > > hvdijk wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > hvdijk wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > For undefs need to push `UndefMaskElem`:
> > > > > > > > > > > > ```
> > > > > > > > > > > > ReuseShuffleIndicies.emplace_back(isa<UndefValue>(V) ? UndefMaskElem : UniqueValues.size());
> > > > > > > > > > > > ```
> > > > > > > > > > > I commented in D126939 that if we use UndefMaskElem when we want undef results, we run into another issue later on where we mistakenly treat a shuffle mask as an identity mask and remove the shuffle.
> > > > > > > > > > But here it should be a case? You're pushing original UndefValue to the UniqueValues vector. So, you have undervalue in the vector and can have UndefMaskElem for it in the mask.
> > > > > > > > > I'm pushing UndefValue in the UniqueValues vector, and its index into ReuseShuffleIndicies, to ensure that that UndefValue I just pushed gets used. If we push UndefMaskElem into ReuseShuffleIndicies (that's what we did before when undef was covered by the earlier `if` block), we may end up getting another value from UniqueValues instead, and that other value from UniqueValues may be poison.
> > > > > > > > A concrete case may be easier to talk about: consider what happens with your idea when we want to build a vector [0, poison, poison, undef]. We get UniqueValues = [0, undef], and ReuseShuffleIndicies = [0, undef, undef, undef]. We then pad UniqueValues to get [0, undef, poison, poison], and interpret the ReuseShuffleIndicies as an identity mask. Thus we actually build a different vector from what we wanted. By preserving the index to get ReuseShuffleIndicies = [0, undef, undef, 1], we get a correct result.
> > > > > > > But it should not be treated as an identity mask since it changes the size. If it does, it is a bug in finalize function. 
> > > > > > It does not change the size? The shuffle's operand (if it would actually be created) would be [0, undef, poison, poison] of size 4, with a shuffle mask of [0, undef, undef, undef], also of size 4.
> > > > > But it should not. This vector should be built as is, without an extra shuffle, just a constant buildvector of the elements in their original order.
> > > > Oh, you're right, I picked a bad example. If all inputs are constant we should end up with a constant, sure. And we also have special logic when we have a single non-constant value. That's separate from the undef/poison problem we're looking at though, please imagine what happens when we try to build [%a, %b, poison, undef] then (and I hope I picked a better example now): we get UniqueValues = [%a, %b, undef], ReuseShuffleIndicies = [0, 1, undef, undef], pad UniqueValues to [%a, %b, undef, poison], interpret [0, 1, undef, undef] as an identity mask, and get a wrong result. By preserving the index for undef we get ReuseShuffleIndicies = [0, 1, undef, 2], which gives a right result.
> > > But here we also do not need a mask, actually, we need to build a vector in its original order, without extra shuffle.
> > That would be better, sure. I don't think it's one we would do for that example (but I could be wrong), and a patch that fixes the poison/undef bug while also optimising that example would be better than this patch. We don't have that yet, but I'm happy to wait a little bit to see if someone (whether you, me, or someone else) can figure out how to do that.
> I can do it tomorrow, if it is ok.
Thanks, much appreciated. If I can figure that out today, I'll update, otherwise I'll wait for you.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127073