[PATCH] D126939: [SLP] Avoid converting undef to poison when gathering.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7530
         std::distance(VL.begin(), find_if(reverse(VL), [](Value *V) {
                                     return !isa<UndefValue>(V);
                                   }).base());
----------------
hvdijk wrote:
> ABataev wrote:
> > hvdijk wrote:
> > > ABataev wrote:
> > > > hvdijk wrote:
> > > > > ABataev wrote:
> > > > > > hvdijk wrote:
> > > > > > > What if we try this the other way around? We have the initialisation of `NumValues` here to find the last non-undef value and then assume we do not need to look beyond that. What if we look for the last non-poison value there, by changing this to `return !isa<PoisonValue>(V);` and treat `undef` as just another constant that needs to be preserved? (The `isa<UndefValue>` inside the next loop would also need to be changed to `isa<PoisonValue>`.) That appears to fix your test just as well, and the impact on other existing SLPVectorizer tests is far less great. Does that also fix your original test?
> > > > > > It won't help, there is still a bug, better just to copy VL in the corner case.
> > > > > Why won't it help? It means we only reach the append of PoisonValue when we actually want to append PoisonValue, so we won't need to fix things up there, does it not?
> > > > UniqueValues may have different order of values than VL, it is not enough just append the tail, also need to reorder values. It is easier just to copy the whole VL.
> > > Yeah, but if we treat UndefValue as just another constant, the logic for handling all other constants already covers that too. I think I may not be explaining accurately what I mean, I can put that up as a separate change later to make it clearer.
> > Yes, possible. Just not sure if we need it. I think it is just 3nough to use original VL in tgis corner case, other cases should be handled by UndefMaskElem.
> Should be, but aren't (at least not always) because when we get to ShuffleInstructionBuilder::finalize, we check ShuffleVectorInst::isIdentityMask which can return true for masks containing undef despite the fact that it's not actually an identity mask, and the correct shufflevector we would otherwise generate gets optimised away, so we again let poison through. We avoid that whole problem if we don't use undef masks to get undef values.
Ah, yes, you're right. Ok, go ahead with your solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126939