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

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

In https://reviews.llvm.org/D46126#1079801, @ABataev wrote:

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


Thanks for taking a look at the patch Alexey!. It does work for the reuse case, but the operations still need to be in transposed form. I'll add a test for this. I already have one reuse test case (build_vec_v4i32_reuse), but this shows reuse in the final binop, not in its operands.



================
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();
+      }))
----------------
ABataev wrote:
> Maybe it's worth it to merge these 2 checks into one to reduce the number of iterations?
Sure, that makes sense.


================
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
----------------
ABataev wrote:
> Maybe it is worth to store the Transposed bundles in the `TreeNode` to not perform this kind of analysis several times?
Yes, that was my thought as well. I'll update the patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D46126