[PATCH] D139718: [SLP][NFC]Inital redesign of ShuffleInstructionBuilder, NFC.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8141-8147
+/// %s1 = shufflevector <2 x > %0, poison, <1 , 0, 1, 0>
+/// %s2 = shufflevector <2 x > %1, poison, <1 , 0, 1, 0>
+/// \endcode
+/// and if need to emit shuffle of %s1 and %s2 with mask <1, 0, 4, 3>, it will
+/// look through %s1 and %s2 and emit
+/// \code
+/// %res = shufflevector <2 x > %0, %1, <0, 1, 2, 3>
----------------
Hm, it looks like there is a mistake in the second example.

`Lets assume %0 -> <x0, x1>
Lets assume %1 -> <y0, y1>
Then these instructions
%s1 = shufflevector <2 x <ty>> %0, poison, <1, 0, 1, 0>
%s2 = shufflevector <2 x <ty>> %1, poison, <1, 0, 1, 0>

yield

%s1 : <x1, x0, x1, x0>
%s2 : <y1, y0, y1, y0>

Then for 
%res = shufflevector <4 x <ty>> %s1, <4 x <ty>> %s2, <1, 0, 4, 3>

<x1, x0, x1, x0><y1, y0, y1, y0>
i,e %res will be <x0, x1 ,y1, x0>
equivalent to 
%res = shufflevector <2 x <ty>> %0, <2 x <ty>> %1, <0, 1, 3, 0>

It looks like second pair is off by one, i.e.  <1, 0, 4, 3> in the example should actually be <1, 0, 5, 4> (or <1, 0, 7, 6>) in order <0,1,2,3> to yield the same result from shuffling the original operands. Right?
`
btw nit: I suggest to change spelling slightly to either look similar to LangRef: <2 x <ty>> or just <2 x ty>
+ there are few places with extra space. 



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8316
+  /// Adds another one input vector and the mask for the shuffling.
+  void add(Value *V1, ArrayRef<unsigned> Order) {
+    SmallVector<int> NewMask;
----------------
This actually looks like a catch. Subtle difference between two methods like their arguments ArrayRef<int> vs ArrayRef<unsigned> actually is not so obvious to imply different semantics. I'd suggest to also name the method differently (basically like it was named earlier).


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8553
 
-  bool NeedToShuffleReuses = !E->ReuseShuffleIndices.empty();
-  unsigned VF = E->getVectorFactor();
-  ShuffleInstructionBuilder ShuffleBuilder(Builder, VF, GatherShuffleExtractSeq,
-                                           CSEBlocks);
+  auto FinalShuffle = [&](Value *V) {
+    ShuffleInstructionBuilder ShuffleBuilder(Builder, *this);
----------------
nit: IMO for better readability it makes sense to add "TreeEntry *E" explicitly as argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139718