[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:
> 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.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6750
       }
+      if (!all_of(GatheredScalars, UndefValue::classof))
+        GatherCost += getGatherCost(GatheredScalars);
----------------
vdmitrie wrote:
> sound like this should be inside of getGatherCost().
No, later all this stuff will go away completely and will be replaced by common procedure for codegen/cost modelling. This is just a temp fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141512