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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2710-2713
+  DenseMap<Value *, SmallPtrSet<User *, 2>> VectorizableValues;
+
+  bool FoundVectorizableValues;
+
----------------
labrinea wrote:
> 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?
Yes, but it is able to repeat this process and build new graphs with the adjusted costs. And we can reuse this cache for other cases, where could not vectorize the graph because of the external costs


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7123-7125
+    if (EU.SkipCost)
+      continue;
+
----------------
labrinea wrote:
> 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?
Nope, for external uses you need to add this cost somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133441