[PATCH] D135876: [InstCombine] Remove redundant splats in InstCombineVectorOps
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 28 12:02:51 PDT 2022
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
More information about the llvm-commits
mailing list