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

[Harald van Dijk via Phabricator via llvm-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126939