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


================
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) &&
----------------
vporpo wrote:
> 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.
Sure, I'm fine with this too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150288