[PATCH] D126939: [SLP] Avoid converting undef to poison when gathering.
Alexey Bataev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jun 5 04:58:13 PDT 2022
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:
> > > > > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126939/new/
https://reviews.llvm.org/D126939
More information about the llvm-commits
mailing list