[PATCH] D135876: [InstCombine] Remove redundant splats in InstCombineVectorOps

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2607-2608
+Instruction *InstCombinerImpl::simplifyBinOpSplats(ShuffleVectorInst &SVI) {
+  if (!match(SVI.getOperand(1), m_Undef()) ||
+      !match(SVI.getShuffleMask(), m_ZeroMask()))
+    return nullptr;
----------------
Use ShuffleVectorInst::isZeroEltSplat() to shorten this? 

We don't necessarily have to match a splat of the 0-element for this transform to work. We can deal with that variant as a follow-up patch to keep things simple for now, but we should eventually handle it.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2617-2621
+  if (!match(BinOpOp0, m_Shuffle(m_Value(X), m_Undef(), m_ZeroMask())))
+    Y = BinOpOp1;
+  if (!match(BinOpOp1, m_Shuffle(m_Value(Y), m_Undef(), m_ZeroMask())))
+    X = BinOpOp0;
+  if (X == nullptr || Y == nullptr)
----------------
I don't think this sequence works the way you are expecting. The 2nd match could overwrite the Y value that was set by the preceding statement.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2625
+  auto Opcode = (Instruction::BinaryOps)cast<Instruction>(BinOp)->getOpcode();
+  Value *ScalarBinOp = Builder.CreateBinOp(Opcode, X, Y);
+  Value *Shuffle =
----------------
This will drop fast-math and other flags, but we should be able to safely propagate those? Try examples with Alive2 to verify that that is correct.


================
Comment at: llvm/test/Transforms/InstCombine/vscale-insert-shuffle-binop.ll:4
+
+target triple = "aarch64-none-eabi"
+
----------------
Remove the target specifier. 

This is target-independent canonicalization, so we don't want target restrictions (and that would likely cause testing bots to fail).


================
Comment at: llvm/test/Transforms/InstCombine/vscale-insert-shuffle-binop.ll:6
+
+define <4 x i8> @splat_binop_splat_x(<4 x i8> %x, <4 x i8> %y) {
+; CHECK-LABEL: @splat_binop_splat_x(
----------------
The fixed vector tests without an extra use are already folded, right? We should add extra uses to all fixed vector tests to show the improvement from this patch. 

Once that's done, please pre-commit the baseline tests, so we'll just see the test diffs in this patch. You can post that as a separate Phab review if there are questions or rebase this patch on top of the preliminary test commit without posting it.


================
Comment at: llvm/test/Transforms/InstCombine/vscale-insert-shuffle-binop.ll:25
+  %ysplat = shufflevector <4 x i8> %y, <4 x i8> poison, <4 x i32> zeroinitializer
+  %b = add <4 x i8> %x, %ysplat
+  %bsplat = shufflevector <4 x i8> %b, <4 x i8> poison, <4 x i32> zeroinitializer
----------------
To get a bit more value from this set of tests, I'd use a unique binop opcode in each test. That way, we know that we're handling non-commutative opcodes, FP opcodes, propagating no-wrap and fast-math flags, etc. as expected.


================
Comment at: llvm/test/Transforms/InstCombine/vscale-insert-shuffle-binop.ll:81
+}
+
+declare void @use(<4 x i8>)
----------------
We still need some negative tests to make sure the transform isn't going overboard:
1. A non-splat shuffle of an input.
2. A non-splat shuffle of the output.
3. A splat that changes the input vector length?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135876