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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev 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()) {
----------------
spatel wrote:
> 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.
Ok, will check it and remove this extra check.


================
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)
----------------
spatel wrote:
> 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.
Yes, I believe so, just added a check to try all possible combinations.
Did not precommit the test initially to be sure that the patch/the test are correct. Will precommit it, if it is fine.


================
Comment at: llvm/test/Transforms/InstCombine/reduction-shufflevector.ll:159
+
+define i32 @reduce_xor_failed(<4 x i32> %x) {
+; CHECK-LABEL: @reduce_xor_failed(
----------------
spatel wrote:
> 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.
Yes, thought about it too, just forgot to add it, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105053