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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288