[llvm] fed0f89 - InstCombine: Negator: don't rely on complexity sorting already being performed (PR47752)
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 7 05:10:14 PDT 2020
Author: Roman Lebedev
Date: 2020-10-07T15:09:50+03:00
New Revision: fed0f890e5698a7a408acaf0aa23319e918f6a2a
URL: https://github.com/llvm/llvm-project/commit/fed0f890e5698a7a408acaf0aa23319e918f6a2a
DIFF: https://github.com/llvm/llvm-project/commit/fed0f890e5698a7a408acaf0aa23319e918f6a2a.diff
LOG: InstCombine: Negator: don't rely on complexity sorting already being performed (PR47752)
In some cases, we can negate instruction if only one of it's operands
negates. Previously, we assumed that constants would have been
canonicalized to RHS already, but that isn't guaranteed to happen,
because of InstCombine worklist visitation order,
as the added test (previously-hanging) shows.
So if we only need to negate a single operand,
we should ensure ourselves that we try constant operand first.
Do that by re-doing the complexity sorting ourselves,
when we actually care about it.
Fixes https://bugs.llvm.org/show_bug.cgi?id=47752
Added:
Modified:
llvm/lib/Transforms/InstCombine/InstCombineInternal.h
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
llvm/test/Transforms/InstCombine/sub-of-negatible.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index eef56c8645f8..10b67695d121 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -762,6 +762,8 @@ class Negator final {
using Result = std::pair<ArrayRef<Instruction *> /*NewInstructions*/,
Value * /*NegatedRoot*/>;
+ std::array<Value *, 2> getSortedOperandsOfBinOp(Instruction *I);
+
LLVM_NODISCARD Value *visitImpl(Value *V, unsigned Depth);
LLVM_NODISCARD Value *negate(Value *V, unsigned Depth);
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
index bfecfe98a590..b321eab01d7a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
@@ -115,6 +115,19 @@ Negator::~Negator() {
}
#endif
+// Due to the InstCombine's worklist management, there are no guarantees that
+// each instruction we'll encounter has been visited by InstCombine already.
+// In particular, most importantly for us, that means we have to canonicalize
+// constants to RHS ourselves, since that is helpful sometimes.
+std::array<Value *, 2> Negator::getSortedOperandsOfBinOp(Instruction *I) {
+ assert(I->getNumOperands() == 2 && "Only for binops!");
+ std::array<Value *, 2> Ops{I->getOperand(0), I->getOperand(1)};
+ if (I->isCommutative() && InstCombiner::getComplexity(I->getOperand(0)) <
+ InstCombiner::getComplexity(I->getOperand(1)))
+ std::swap(Ops[0], Ops[1]);
+ return Ops;
+}
+
// FIXME: can this be reworked into a worklist-based algorithm while preserving
// the depth-first, early bailout traversal?
LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {
@@ -159,11 +172,13 @@ LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {
// In some cases we can give the answer without further recursion.
switch (I->getOpcode()) {
- case Instruction::Add:
+ case Instruction::Add: {
+ std::array<Value *, 2> Ops = getSortedOperandsOfBinOp(I);
// `inc` is always negatible.
- if (match(I->getOperand(1), m_One()))
- return Builder.CreateNot(I->getOperand(0), I->getName() + ".neg");
+ if (match(Ops[1], m_One()))
+ return Builder.CreateNot(Ops[0], I->getName() + ".neg");
break;
+ }
case Instruction::Xor:
// `not` is always negatible.
if (match(I, m_Not(m_Value(X))))
@@ -344,16 +359,18 @@ LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {
ConstantExpr::getShl(Constant::getAllOnesValue(Op1C->getType()), Op1C),
I->getName() + ".neg");
}
- case Instruction::Or:
+ case Instruction::Or: {
if (!haveNoCommonBitsSet(I->getOperand(0), I->getOperand(1), DL, &AC, I,
&DT))
return nullptr; // Don't know how to handle `or` in general.
+ std::array<Value *, 2> Ops = getSortedOperandsOfBinOp(I);
// `or`/`add` are interchangeable when operands have no common bits set.
// `inc` is always negatible.
- if (match(I->getOperand(1), m_One()))
- return Builder.CreateNot(I->getOperand(0), I->getName() + ".neg");
+ if (match(Ops[1], m_One()))
+ return Builder.CreateNot(Ops[0], I->getName() + ".neg");
// Else, just defer to Instruction::Add handling.
LLVM_FALLTHROUGH;
+ }
case Instruction::Add: {
// `add` is negatible if both of its operands are negatible.
SmallVector<Value *, 2> NegatedOps, NonNegatedOps;
@@ -383,26 +400,29 @@ LLVM_NODISCARD Value *Negator::visitImpl(Value *V, unsigned Depth) {
return Builder.CreateSub(NegatedOps[0], NonNegatedOps[0],
I->getName() + ".neg");
}
- case Instruction::Xor:
+ case Instruction::Xor: {
+ std::array<Value *, 2> Ops = getSortedOperandsOfBinOp(I);
// `xor` is negatible if one of its operands is invertible.
// FIXME: InstCombineInverter? But how to connect Inverter and Negator?
- if (auto *C = dyn_cast<Constant>(I->getOperand(1))) {
- Value *Xor = Builder.CreateXor(I->getOperand(0), ConstantExpr::getNot(C));
+ if (auto *C = dyn_cast<Constant>(Ops[1])) {
+ Value *Xor = Builder.CreateXor(Ops[0], ConstantExpr::getNot(C));
return Builder.CreateAdd(Xor, ConstantInt::get(Xor->getType(), 1),
I->getName() + ".neg");
}
return nullptr;
+ }
case Instruction::Mul: {
+ std::array<Value *, 2> Ops = getSortedOperandsOfBinOp(I);
// `mul` is negatible if one of its operands is negatible.
Value *NegatedOp, *OtherOp;
// First try the second operand, in case it's a constant it will be best to
// just invert it instead of sinking the `neg` deeper.
- if (Value *NegOp1 = negate(I->getOperand(1), Depth + 1)) {
+ if (Value *NegOp1 = negate(Ops[1], Depth + 1)) {
NegatedOp = NegOp1;
- OtherOp = I->getOperand(0);
- } else if (Value *NegOp0 = negate(I->getOperand(0), Depth + 1)) {
+ OtherOp = Ops[0];
+ } else if (Value *NegOp0 = negate(Ops[0], Depth + 1)) {
NegatedOp = NegOp0;
- OtherOp = I->getOperand(1);
+ OtherOp = Ops[1];
} else
// Can't negate either of them.
return nullptr;
diff --git a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
index 92d1ac225a01..3d988c9f3d4a 100644
--- a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
+++ b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
@@ -1239,5 +1239,29 @@ define i4 @negate_freeze_extrause(i4 %x, i4 %y, i4 %z) {
ret i4 %t2
}
+; Due to the InstCombine's worklist management, there are no guarantees that
+; each instruction we'll encounter has been visited by InstCombine already.
+; In particular, most importantly for us, that means we have to canonicalize
+; constants to RHS ourselves, since that is helpful sometimes.
+; This used to cause an endless combine loop.
+define void @noncanonical_mul_with_constant_as_first_operand() {
+; CHECK-LABEL: @noncanonical_mul_with_constant_as_first_operand(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[IF_END:%.*]]
+; CHECK: if.end:
+; CHECK-NEXT: br label [[IF_END]]
+;
+entry:
+ br label %if.end
+
+if.end:
+ %e.0 = phi i32 [ undef, %entry ], [ %div, %if.end ]
+ %conv = trunc i32 %e.0 to i16
+ %mul.i = mul nsw i16 -1, %conv
+ %conv1 = sext i16 %mul.i to i32
+ %div = sub nsw i32 0, %conv1
+ br label %if.end
+}
+
; CHECK: !0 = !{!"branch_weights", i32 40, i32 1}
!0 = !{!"branch_weights", i32 40, i32 1}
More information about the llvm-commits
mailing list