[PATCH] D139718: [SLP][NFC]Inital redesign of ShuffleInstructionBuilder, NFC.
Alexey Bataev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 9 09:44:12 PST 2022
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
More information about the llvm-commits
mailing list