[PATCH] D133441: [SLP] Vectorize mutual horizontal reductions.

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2710-2713
+  DenseMap<Value *, SmallPtrSet<User *, 2>> VectorizableValues;
+
+  bool FoundVectorizableValues;
+
----------------
ABataev wrote:
> Not sure this is the best option to keep external vectorizable values, again it does not allow to handle values across different graphs, only across the same instance.
I am not sure I am following your line of thought. As far as I understand the vectorizer keeps one active graph at a time, created by `buildTree()`, which either gets vectorized or destroyed and then another bundle of instructions is scheduled and so on. The DenseMap I added works as a cache of tree entries that failed to vectorize due to extraction cost. Therefore it holds values across different graphs if that makes sense. As I said earlier, the change triggers in quite a few unit tests if the conditions are relaxed a bit, but I couldn't tell for sure if the new codegen was better or worse than before. If that's not what you meant, can you please explain and maybe suggest an alternative approach?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7123-7125
+    if (EU.SkipCost)
+      continue;
+
----------------
ABataev wrote:
> This again is too optimistic, you need to try to estimate the cost of extra shuffles, required for the vectorization. 
I thought that the shuffle cost is already accounted inside `getEntryCost()`, am I wrong? Or maybe that logic needs to be patched too?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11320
+          if (V.foundVectorizableValues())
+            return ReductionRoot;
           if (!AdjustReducedVals())
----------------
ABataev wrote:
> Why return?
Because returning a non null value means the result will be put back in the queue and rescheduled later, which is what we want for trees that failed due to extraction cost. See line 11831 below.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11831
         // Try to find another reduction.
         Stack.emplace(I, Level);
         continue;
----------------
When `I == Inst` here it means that `tryToReduce()` returned the root of the horizontal reduction and not a newly vectorized tree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133441