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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
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) {
----------------
spatel wrote:
> 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.
I think it was, yes. But mostly just because it simplified things and it didn't sound as profitable (as-in the best order to aim for is an identity mask and having extra lanes in there can mess that up). There is a test for it in reduceshuffle_twoin_repeat_v4i32.  I was aiming for something that removed the shuffle - either because it can be removed or it becomes a concat or something simple like that.

It might be profitable in more cases though, and so long as we are cost modelling it, sorting the mask indices should be OK. I can change it to that, as it has the added benefit that is simplifies the code a little.


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

https://reviews.llvm.org/D123494