[PATCH] D150288: [SLP] Simplify `tryToVectorizeSequence()`

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14342
+    // Try the full Seeds vector first.
+    if (TryToVectorizeHelper(Seeds, /*MaxVFOnly=*/false)) {
       Changed = true;
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > What I'm afraid is that it may prevent some full-register optimizations. You will early optimize part register tress and miss some full-register trees (probably, with alternate opcodes)
> > > So would you prefer iterating over the SeedMap twice?
> > > Once with `MaxVFOnly = true` and then once with `MaxVFOnly = false`?
> > Yes, that's the idea
> The problem is that neither is clearly the better strategy. You can come up with examples where vectorizing for MaxVF first across all seeds leads to worse vectorization than trying to vectorize each seed multiple times starting from MaxVF all the way to MinVF. For example when you end up with a short wide tree versus a deep narrower tree. 
> Also if we are to follow a strategy MaxVF first, then it would probably make more sense if we visit seeds based on some specific order, e.g. from largest to smallest, and even then there would still be cases when the strategy won't work.
> 
> In such cases I am in favor of keeping the code as simple as possible because a more complicated solution won't necessarily pay off.
I prefer to keep original implementation for the 2 reasons.
1. I plan to add a new node entry, which may improve vectorization of nodes with the different operands.
2. Need to try to improve the whole process by trying vectorization not only for tail values, but for other too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288