[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: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
----------------
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.



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


================
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);
----------------
description?


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


================
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)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139718