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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8119
+/// Merges shuffle masks and emits final shuffle instruction, if required. It
+/// supports shuffling of 2 input vectors. It implements lazy shuffles emission,
+/// when the actual shuffle instruction is generated only if this is actually
----------------
vdmitrie wrote:
> Could you please add more details on restrictions assumed on masks and input vectors? I mean sizes of masks and vectors.
> The description will greatly benefit from an example.
> 
There will be no limitations on masks and vectors. What kind of example do you think might be useful here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8127
+  SmallVector<int> CommonMask;
+  SmallVector<Value *, 2> InVectors;
+  IRBuilderBase &Builder;
----------------
vdmitrie wrote:
> Please describe the purpose of CommonMask and InVectors data?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8145
+    ~ShuffleIRBuilder() = default;
+    Value *createShuffleVector(Value *V1, Value *V2, ArrayRef<int> Mask) {
+      Value *Vec = Builder.CreateShuffleVector(V1, V2, Mask);
----------------
vdmitrie wrote:
> description?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8153
+    }
+    Value *createShuffleVector(Value *V1, ArrayRef<int> Mask) {
+      if (Mask.empty())
----------------
vdmitrie wrote:
> description?
OK


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8245
+          V = createShuffle(InVectors.front(), nullptr, CommonMask);
+          for (unsigned Idx = 0, Sz = CommonMask.size(); Idx < Sz; ++Idx)
+            if (CommonMask[Idx] != UndefMaskElem)
----------------
vdmitrie wrote:
> this code is repeating. Seems like it makes sense to put additional efforts to cleanup it a bit. Can we sink it past the if-else blocks and fuse with loop at 8249?
I think it will be much harder to understand this complex code, if these 2 loops are fused. I'll try to outline common part to lambdas/functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139718