[PATCH] D46126: [SLP] Vectorize transposable binary operand bundles

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

Will it work correctly if some of the operations are used several times in the bundles? It would be good to have the tests for this kind of situation.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1437-1453
+  // Check if the operand-zero bundle is isomorphic. If this is the case, a
+  // transpose is unnecessary since the bundle is already vectorizable.
+  if (all_of(VL.drop_front(), [VL0Op0](Value *V) {
+        auto *VLI = cast<BinaryOperator>(V);
+        return cast<Instruction>(VLI->getOperand(0))->getOpcode() ==
+               VL0Op0->getOpcode();
+      }))
----------------
Maybe it's worth it to merge these 2 checks into one to reduce the number of iterations?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1484
+  SmallVector<BoUpSLP::ValueList, 4> Bundles;
+  for (auto &Entry : Opcode2Operands) {
+    ArrayRef<Value *> Bundle(Entry.second);
----------------
`auto &`->`const BoUpSLP::ValueList &` or, probably, you may use `ArrayRef<Value*` here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1488
+      ArrayRef<Value *> Slice = Bundle.take_front(MinSize);
+      Bundles.push_back(BoUpSLP::ValueList(Slice.begin(), Slice.end()));
+      Bundle = Bundle.drop_front(MinSize);
----------------
Try to use `Bundles.emplace_back(Slice.begin(), Slice.end());`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1975
+      if (isa<BinaryOperator>(VL0))
+        if (auto TransposedBundles = transposeBinOpBundle(VL)) {
+          for (auto &Bundle : *TransposedBundles)
----------------
`auto`->`Optional<SmallVector<BoUpSLP::ValueList, 4>>`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1976
+        if (auto TransposedBundles = transposeBinOpBundle(VL)) {
+          for (auto &Bundle : *TransposedBundles)
+            buildTree_rec(Bundle, Depth + 1, UserTreeIdx);
----------------
`auto &`->`const auto &`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2478
+      unsigned TransposeShuffleCost = 0;
+      SmallVector<const Value *, 4> Operands;
+      if (auto TransposedBundles = transposeBinOpBundle(E->Scalars)) {
----------------
Pre-reserve the space for the Operands here.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2479
+      SmallVector<const Value *, 4> Operands;
+      if (auto TransposedBundles = transposeBinOpBundle(E->Scalars)) {
+        // If this binary operator's operand bundles can be transposed, we need
----------------
Maybe it is worth to store the Transposed bundles in the `TreeNode` to not perform this kind of analysis several times?


Repository:
  rL LLVM

https://reviews.llvm.org/D46126