[PATCH] D140100: [SLP]Integrate looking through shuffles logic into ShuffleInstructionBuilder.

[Alexey Bataev via Phabricator via llvm-commits]

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