[instcombine] Delete dead transform for reverse of binop

[preames]

We canonicalize reverse to after a binop in foldVectorBinop, and simplify reverse pairs in InstSimplify, so these elimination transforms are redundant.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Philip Reames (preames)

Changes

We canonicalize splats to RHS, so this transform should be dead code.

---

Full diff: https://github.com/llvm/llvm-project/pull/143967.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (-6)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index c169ab25b2106..d5738520715b0 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -3565,12 +3565,6 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
                                              OldBinOp, OldBinOp->getName(),
                                              II->getIterator()));
       }
-      // rev(binop BO0Splat, rev(Y)) --> binop BO0Splat, Y
-      if (match(BO1, m_VecReverse(m_Value(Y))) && isSplatValue(BO0))
-        return replaceInstUsesWith(CI,
-                                   BinaryOperator::CreateWithCopiedFlags(
-                                       OldBinOp->getOpcode(), BO0, Y, OldBinOp,
-                                       OldBinOp->getName(), II->getIterator()));
     }
     // rev(unop rev(X)) --> unop X
     if (match(Vec, m_OneUse(m_UnOp(m_VecReverse(m_Value(X)))))) {

[dtcxzyw]

We canonicalize splats to RHS

Does it still happen when BO0splat is not a constant? I cannot find related code in InstCombinerImpl::foldVectorBinop.
BTW, the binop may not be commutative. I think it is still useful for sub/div/shl.

[preames]

We canonicalize splats to RHS

Does it still happen when BO0splat is not a constant? I cannot find related code in InstCombinerImpl::foldVectorBinop. BTW, the binop may not be commutative. I think it is still useful for sub/div/shl.

You're right about the canonicalization (and the commutative bit), but interestingly, it appears this code still has no effect. We have test cases (e.g. reverse_binop_reverse_splat_LHS) for exactly the cases you pointed to, and even without this bit of code, everything continues to work. We appear to have another transform which given binop splat X, rev(Y) canonicalizes this as rev(binop splat X, Y), and then a rev(rev(x)) -> X transform kills the pair of reverse.

(See foldVectorBinOp, and createBinOpReverse)

It looks like this entire case in the switch might be dead.

Unless there's some way to trigger this I'm not seeing? We don't have any test coverage which is impacted by it, is there some case we should be testing here?

[nikic]

You might be able to construct something where the reverses are multi-use? Though I wouldn't keep the code around just for that...