[PATCH] D140100: [SLP]Integrate looking through shuffles logic into ShuffleInstructionBuilder.
Alexey Bataev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 16 10:39:32 PST 2022
ABataev added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6306
+ /// Tries to combine 2 different masks into single one.
+ static void combineMasks(unsigned LocalVF, SmallVectorImpl<int> &Mask,
+ ArrayRef<int> ExtMask) {
----------------
vdmitrie wrote:
> Please describe the purpose of LocalVF argument?
Will do
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6342
+ /// and if need to emit shuffle of %s1 and %s2 with mask <1, 0, 5, 4>, it will
+ /// look through %s1 and %s2 and emit
+ /// \code
----------------
vdmitrie wrote:
> Thanks for the update. What is still confusing me...
> The comment says "emit" while this method belongs to an analysis class, i.e. is not supposed to emit anything. For these two examples input value of argument V presumable will be %s2 (what will be the Mask argument value on input by the way? the mask from %s2 instruction?), and on output V presumably points to %0 with the mask having value <0,1,2,3>. Is that correct?
> Could you please add this kind of clarification to the comment?
> btw the boolean return value is still not described.
I'll fix it
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6384
+ if (SV->isZeroEltSplat()) {
+ IdentityOp = SV;
+ IdentityMask.assign(Mask);
----------------
vdmitrie wrote:
> What is still confusing.. it is unclear why broadcast mask & Op saved as identity (it's not actually an identity mask).
> Probably the variable name does not reflect accurately its purpose...
>
Because we can treat it as identity too, because the mask 0,1,2,3,.. can be applied to broadcasts too.
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6449
+ template <typename ShuffleBuilderTy>
+ static Value *createShuffle(Value *V1, Value *V2, ArrayRef<int> Mask,
+ ShuffleBuilderTy &Builder) {
----------------
vdmitrie wrote:
> Could you share some insights why you put this into BaseShuffleAnalysis class method rather than into ShuffleInstructionBuilder? Since it produces new code technically it is not a pure analysis method. And why you want it to be a template (it seems to be instantiated just once with ShuffleIRBuilder)?
It will be used for ShuffleCostEstimator too to avoid duplication/diffs in analysis/emission and cost estimation logic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140100/new/
https://reviews.llvm.org/D140100
More information about the llvm-commits
mailing list