[PATCH] D141512: [SLP]Improve isGatherShuffledEntry by looking deeper through the reused scalars.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6713-6717
+    if (!(E->getOpcode() == Instruction::Load && !E->isAltShuffle() &&
+          !all_of(E->Scalars, [this](Value *V) { return getTreeEntry(V); }) &&
+          !isSplat(E->Scalars)  &&
+          (E->Scalars == GatheredScalars || GatheredScalars.size() > 2)))
+      GatherShuffle = isGatherShuffledEntry(E, GatheredScalars, Mask, Entries);
----------------
vdmitrie wrote:
> ABataev wrote:
> > vdmitrie wrote:
> > > I find this "if" condition quite difficult to follow. 
> > > I tried to simplify it by applying logical NOT into expressions in parens
> > >  and got the following (which, if  I'm not mistaken, is equivalent):
> > > 
> > >      if (E->getOpcode() != Instruction::Load || E->isAltShuffle() ||
> > >          all_of(E->Scalars, [this](Value *V) { return getTreeEntry(V); }) ||
> > >          isSplat(E->Scalars) ||
> > >          (E->Scalars != GatheredScalars && GatheredScalars.size() <= 2))
> > > 
> > > where I still don't quite follow the second part.
> > > Besides,  what makes me nervous is use of isAltShuffle() interface here which was not originally designed for gather nodes.  We can gather for multiple reasons including inability to schedule a bundle. We just leave this information not cleared when generate a gather node for such bundle. But originally we only used it to emulate binary operations not present in a hardware to vectorize a set of scalars.
> > > As I understand you are trying to pre-filter gather nodes probably trying to find reshuffles of maximum two distinct scalar instructions before doing further analysis and use isAltShuffle() as an indicator for two ops specifically. But shouldn't we clear instructions state for irrelevant cases when we build a tree
> > > if we are not going to use these nodes for further re-analysis?
> > > We probably could write a reason why scalars are being gathered (directly into a node or as a sidecar data). That would likely simplify many places where we deal with re-analysis to restore that information after we have already built a tree. 
> > > 
> > > 
> > > 
> > > 
> > 1. Even gather nodes may have isAltShuffle(). It may happen, if some vectorizable nodes with isAltShuffle() cannot be scheduled for some reason.
> > Not sure it should be cleared, we can do this, but actually it won't help with anything, just increase the codebase.
> > 2. This check here is just be safe with the loads nodes, which may be split into several vector loads, splat nodes, and for very small shuffles, which are definitely not profitable.
> Thanks for explanation. w.r.t. isAltShuffle, yep, probably clearing out the data is not really necessary.
> I just highlighted that use of the interface may be confusing. It is just a coincidence that it gives you the answer you'd like to have here, but it was not designed specifically for that. So if you add a short note about that as a comment that would definitely be helpful.
> 
> What the following part of the "if" condition targets?
> E->Scalars != GatheredScalars && GatheredScalars.size() <= 2
> 
> The first part makes "true" if they turn out reordered, the second - when we have just two scalars (for one that would give a splat, for which we have check earlier). Is that reordering check is how you rule out  unprofitable cases? I just do not understand why isAltShuffle() would not hit. Unprofitable cases still may have it set.
isAltShuffle works only for instructions, it does not work for other cases like arguments, globals, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141512