[PATCH] D105053: [INSTCOMBINE] Transform reduction(shuffle V, poison, unique_mask) to reduction(V).

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2013
+          (match(Arg, m_Shuffle(m_Value(V), m_Undef(), m_Mask(Mask))) ||
+           match(Arg, m_Shuffle(m_Undef(), m_Value(V), m_Mask(Mask)))) &&
+          cast<ShuffleVectorInst>(Arg)->isSingleSource()) {
----------------
We canonicalize undef/poison operand as the 2nd input vector and we're within InstCombine, so checking for that is unnecessary?

https://github.com/llvm/llvm-project/blob/b458bb8c04cd5ed025884d424f386a00c9c6857e/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp#L2427

The tests have coverage now to verify that unless I missed something in the earlier draft.


================
Comment at: llvm/test/Transforms/InstCombine/reduction-shufflevector.ll:9
+;
+  %shuf = shufflevector <4 x i32> %x, <4 x i32> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
+  %res = call i32 @llvm.vector.reduce.add.v4i32(<4 x i32> %shuf)
----------------
Identity shuffle should be reduced even without this patch.
It would be better to pre-commit the test file, so we can make sure that we are testing as expected.


================
Comment at: llvm/test/Transforms/InstCombine/reduction-shufflevector.ll:159
+
+define i32 @reduce_xor_failed(<4 x i32> %x) {
+; CHECK-LABEL: @reduce_xor_failed(
----------------
It's independent of this patch, but we might want to mark this and similar tests with a TODO - if a reduction has a known undef/poison element in the input, the result can be simplified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105053