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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo marked an inline comment as done.
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:
> 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`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:14361-14365
-      auto GetMinNumElements = [&R](Value *V) {
-        unsigned EltSize = R.getVectorElementSize(V);
-        return std::max(2U, R.getMaxVecRegSize() / EltSize);
-      };
-      if (NumElts < GetMinNumElements(*IncIt) &&
----------------
ABataev wrote:
> vporpo wrote:
> > ABataev wrote:
> > > Does it make sense to drop this check?
> > Isn't this already checked within `TryToVectorizeHelper`? If I am not mistaken both `vectorizeStores()` and `tryToVectorizeList()` already check for minimum VF.
> It just early skips not profitable cases, saves some compile time, checks that vector can be fed with the elements, otherwise it does not worth trying vectorization, it will be scalarized.
It might save a bit of compilation time, but it is just duplicate code. I think it is probably better to move the checks in `vectorizeStores()` and `tryToVectorizeList()` as early as possible. Let me check if this is possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288