[PATCH] D98714: [SLP] Add insertelement instructions to vectorizable tree

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4215-4217
+    // No extract cost for vector "scalar"
+    if (isa<FixedVectorType>(EU.Scalar->getType()))
+      continue;
----------------
anton-afanasyev wrote:
> ABataev wrote:
> > anton-afanasyev wrote:
> > > ABataev wrote:
> > > > anton-afanasyev wrote:
> > > > > ABataev wrote:
> > > > > > I don't think this is correct. I think you need to use code from my patch D101555 for better cost estimation here.
> > > > > Why? We don't generate any extract instructions for external using of vector, so no need to cost it.
> > > > > 
> > > > > I think you mixed two different cases up.
> > > > > The patch D101555 you referenced is about cost estimation when insert is _user_, but here is the case when insert is _used_. We do not really need to "extract" it, since its user uses vector value rather than scalar one.
> > > > > 
> > > > > Also I don't think we need to use code from D101555 in this patch, since it does the same by the other way. The main idea of this patch is to unify the way we process inserts (the only vector tree node for now) vs ordinary tree nodes. The inserts are tree node now, and they are sorted by index, so no need to shuffle them. We could use `ReorderIndicies` if needed, but no need, since operands are sorted as well.
> > > > I think you're missing shuffle cost here. If the external user is a vector and extracted element is inserted into the different lane - it is at least shuffle and need to add its cost.
> > > > I think you're missing shuffle cost here. 
> > > The cost of what shuffle? We don't generate any shuffle.
> > > 
> > > > If the external user is a vector ...
> > > Not "the external user is a vector", but its operand is a vector. We do not need to extract any special lane, since _whole_ vector is using and replacing after "vectorization". In this special case (when tree node is inserts) we have vector "scalars" (inserts have vector type) and their "vectorization" is just removing (i.e. replacing by vectorized operands).
> > Ok, I see. You correctly excluded the cost of the final (sub)vector reuse.
> > But I suggest to improve the cost model for inserts.
> > ```
> > i1 = insertelement undef, v0
> > i2 = insertelement i1, v1
> > ....
> > ii1 = insertelement undef, v1
> > ```
> > If <v0,v1> gets vectorized, you need to count the cost of the extract of `v1`. Instead, we can count it as shuffle and build the final shuffle. It can be much more profitable than relying on extract element costs/instructions. But probably this can be addressed in the next patch.
> Ok, I see you too. Though I don't see how this case is addressed within D101555, two inserts `i1` and `ii1` cannot be cought to the same tree and they don't occur in one `InsertUses` therefore.
> 
> Anyway, I suggest to address this in the separate patch. It's rather rare case in natural life.
Actually, D101555 addresses exactly described problem. In some cases, it really improves the performance, especially for SSE/AVX/AVX2 targets. I will update it once this patch is landed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98714