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

[Harald van Dijk via Phabricator via llvm-commits]

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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127073