[PATCH] D123494: [VectorCombine] Find and remove shuffles from commutative reductions

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added a comment.

LGTM



================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1162
+    if (auto *CI = dyn_cast<Instruction>(CV)) {
+      if (CI->isBinaryOp()) {
+        for (auto *Op : CI->operand_values())
----------------
dmgreen wrote:
> samtebbs wrote:
> > Does this also need to check if the binary operation is commutative? It may be a good idea to add a test with a non-commutative reduction and a qualifying shuffle, to make sure this function's behaviour isn't broken in the future.
> I don't think it should matter if the binary operator is commutative. There is a test with a lshr in reduceshuffle_twoin_ext_v16i32, for example. These binary operators we are only looking through, back to the shuffle. We don't modify them directly, just the order that the vector-lanes operate. Which is safe because they are only used by the reduction, and only use splats and the shuffle we transform.
Yes you're right. Only the reduction intrinsic needs to be commutative which you have checked for in the switch statement above.


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

https://reviews.llvm.org/D123494