[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:6384
+      if (SV->isZeroEltSplat()) {
+        IdentityOp = SV;
+        IdentityMask.assign(Mask);
----------------
ABataev wrote:
> 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.
A small example would be very helpful here...


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6449
+  template <typename ShuffleBuilderTy>
+  static Value *createShuffle(Value *V1, Value *V2, ArrayRef<int> Mask,
+                              ShuffleBuilderTy &Builder) {
----------------
ABataev wrote:
> 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.
Ah, I see. There is actually a comment to the class saying that, which I overlooked. So you are going to produce another class from it which will serve as cost estimator. Perhaps having "Analysis" as a part of its name isn't quite accurate. It probably makes sense to rename it to something like ShuffleBuilderBase to avoid confusion in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140100