[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:2609
+  Value *Op0 = SVI.getOperand(0);
+  Value *X = nullptr, *Y = nullptr;
+  if (!match(Op0, m_BinOp(m_Shuffle(m_Value(X), m_Undef(), m_ZeroMask()),
----------------
It's easier to spot a coding mistake if we don't initialize these; that's because the compiler can detect a bogus use of an uninitialized value and put up a warning.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2620
+  BinaryOperator *NewBinOp =
+      cast<BinaryOperator>(Builder.CreateBinOp(BinOp->getOpcode(), X, Y));
+  if (isa<FPMathOperator>(BinOp))
----------------
This naked cast<> isn't safe (although I'm not sure how to create a test case for it).

You probably want to do this instead:
  Value *NewBO = Builder.CreateBinOp(cast<BinaryOperator>(Op0)->getOpcode(), X, Y);
  if (auto *NewBOI = dyn_cast<Instruction>(NewBO))
    NewBOI->copyIRFlags(&I);

But we should have tests that include FMF, nsw, nuw etc to verify this is happening as expected.


================
Comment at: llvm/test/Transforms/InstCombine/shuffle-binop.ll:75
   %b = sub <4 x i8> %x, %ysplat
   %bsplat = shufflevector <4 x i8> %b, <4 x i8> poison, <4 x i32> zeroinitializer
   ret <4 x i8> %bsplat
----------------
Alter this test to use at least one poison/undef element in the mask for %bsplat, so we'll verify that the mask propagates as expected.


================
Comment at: llvm/test/Transforms/InstCombine/shuffle-binop.ll:112
 ; CHECK-LABEL: @vscale_splat_binop_splat_y(
-; CHECK-NEXT:    [[YSPLAT:%.*]] = shufflevector <vscale x 4 x i32> [[Y:%.*]], <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer
-; CHECK-NEXT:    [[B:%.*]] = sdiv <vscale x 4 x i32> [[X:%.*]], [[YSPLAT]]
-; CHECK-NEXT:    [[BSPLAT:%.*]] = shufflevector <vscale x 4 x i32> [[B]], <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer
-; CHECK-NEXT:    ret <vscale x 4 x i32> [[BSPLAT]]
+; CHECK-NEXT:    [[TMP1:%.*]] = sdiv <vscale x 4 x i32> [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[TMP2:%.*]] = shufflevector <vscale x 4 x i32> [[TMP1]], <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer
----------------
Oops - that's actually a miscompile. (You can confirm with Alive2 and a simpler fixed vector type.) 

We can't do this transform in general with integer division or remainder opcodes because those have the potential for immediate UB.

Keep the tests, so we know we don't have this bug, but add tests with other opcodes (and IR flags) to positively show the transform.

The predicate needed to guard the transform is likely:
  isSafeToSpeculativelyExecute(BinOp))



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

https://reviews.llvm.org/D135876