[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:7123-7125
+    if (EU.SkipCost)
+      continue;
+
----------------
labrinea wrote:
> ABataev wrote:
> > 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.
> Another thing I tried was to cache the result of `getEntryCost()` for the tree entries that correspond to the external users, and then use that cost here instead of the extraction cost estimate. I suppose that's not correct either judging from the produced codegen. I am not familiar with the SLP vectorizer and so my ideas are based on intuition rather than actual understanding I am affraid. I'd be open for an offline discussion if you think that would be enlightening/beneficial.
> 
> I am not entirely sure but it doesn't seem possible to predict the extraction cost without actually vectorizing first and then retrospectively reflecting upon it. While reading the debug output I noticed that insert/extract-element code is being added when vectorizing. Then this code either gets fully vectorized and removed, in which case the cost estimate ought to be zero, or it remains in the final codegen.
You cannot estimate it when you see the instruction for the first time, but if it is the second time, you can have the info already about its position in the previous tree/graph. And thus you can get info about possible extra shuffles.

Also, you should be careful with extractelement instructions emitting, they may led to gather nodes and again too optimistic cost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133441