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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie 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) {
----------------
Please describe the purpose of LocalVF argument?


================
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
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6384
+      if (SV->isZeroEltSplat()) {
+        IdentityOp = SV;
+        IdentityMask.assign(Mask);
----------------
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...



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6449
+  template <typename ShuffleBuilderTy>
+  static Value *createShuffle(Value *V1, Value *V2, ArrayRef<int> Mask,
+                              ShuffleBuilderTy &Builder) {
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140100