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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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())
----------------
samtebbs wrote:
> 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.
Could add a near duplicate of that test except the shuffled value is operand 1 of the shift  (to prove that we can find the shuffle through either operand of a binop).


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1213-1219
+  int NumUsed1 = UsedLanes1.find_first_unset();
+  if (NumUsed1 == -1)
+    NumUsed1 = NumInputElts;
+  int NumUsed2 = UsedLanes2.find_first_unset();
+  if (NumUsed2 == -1)
+    NumUsed2 = NumInputElts;
+  if (NumUsed1 + NumUsed2 == NumVecElts) {
----------------
If I'm seeing it correctly, this logic ensures we won't try to re-shuffle a mask with duplicate elements like:
<i32 1, i32 2, i32 1, i32 3>

Is that intentional? Either way, it would be good to have a test like that.


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

https://reviews.llvm.org/D123494