-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release/18.x: [InstCombine] Fix miscompile in negation of select (#89698) #91089
Conversation
Swapping the operands of a select is not valid if one hand is more poisonous that the other, because the negation zero contains poison elements. Fix this by adding an extra parameter to isKnownNegation() to forbid poison elements. I've implemented this using manual checks to avoid needing four variants for the NeedsNSW/AllowPoison combinations. Maybe there is a better way to do this... Fixes llvm#89669. (cherry picked from commit a1b1c4a)
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: AtariDreams (AtariDreams) ChangesSwapping the operands of a select is not valid if one hand is more poisonous that the other, because the negation zero contains poison elements. Fix this by adding an extra parameter to isKnownNegation() to forbid poison elements. I've implemented this using manual checks to avoid needing four variants for the NeedsNSW/AllowPoison combinations. Maybe there is a better way to do this... Fixes #89669. (cherry picked from commit a1b1c4a) Full diff: https://github.com/llvm/llvm-project/pull/91089.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 7360edfce1f39a..a5fa0c8a2c74c8 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -134,7 +134,8 @@ bool isKnownNonZero(const Value *V, const DataLayout &DL, unsigned Depth = 0,
/// Currently can recoginze Value pair:
/// 1: <X, Y> if X = sub (0, Y) or Y = sub (0, X)
/// 2: <X, Y> if X = sub (A, B) and Y = sub (B, A)
-bool isKnownNegation(const Value *X, const Value *Y, bool NeedNSW = false);
+bool isKnownNegation(const Value *X, const Value *Y, bool NeedNSW = false,
+ bool AllowPoison = true);
/// Returns true if the give value is known to be non-negative.
bool isKnownNonNegative(const Value *V, const SimplifyQuery &SQ,
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 9f9451e4e814ac..72b1c97d20204d 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -7621,17 +7621,27 @@ static SelectPatternResult matchMinMax(CmpInst::Predicate Pred,
return {SPF_UNKNOWN, SPNB_NA, false};
}
-bool llvm::isKnownNegation(const Value *X, const Value *Y, bool NeedNSW) {
+bool llvm::isKnownNegation(const Value *X, const Value *Y, bool NeedNSW,
+ bool AllowPoison) {
assert(X && Y && "Invalid operand");
- // X = sub (0, Y) || X = sub nsw (0, Y)
- if ((!NeedNSW && match(X, m_Sub(m_ZeroInt(), m_Specific(Y)))) ||
- (NeedNSW && match(X, m_NSWSub(m_ZeroInt(), m_Specific(Y)))))
+ auto IsNegationOf = [&](const Value *X, const Value *Y) {
+ if (!match(X, m_Neg(m_Specific(Y))))
+ return false;
+
+ auto *BO = cast<BinaryOperator>(X);
+ if (NeedNSW && !BO->hasNoSignedWrap())
+ return false;
+
+ auto *Zero = cast<Constant>(BO->getOperand(0));
+ if (!AllowPoison && !Zero->isNullValue())
+ return false;
+
return true;
+ };
- // Y = sub (0, X) || Y = sub nsw (0, X)
- if ((!NeedNSW && match(Y, m_Sub(m_ZeroInt(), m_Specific(X)))) ||
- (NeedNSW && match(Y, m_NSWSub(m_ZeroInt(), m_Specific(X)))))
+ // X = -Y or Y = -X
+ if (IsNegationOf(X, Y) || IsNegationOf(Y, X))
return true;
// X = sub (A, B), Y = sub (B, A) || X = sub nsw (A, B), Y = sub nsw (B, A)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
index 62e49469cb0198..beb404bbdc0166 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
@@ -320,7 +320,8 @@ std::array<Value *, 2> Negator::getSortedOperandsOfBinOp(Instruction *I) {
return NegatedPHI;
}
case Instruction::Select: {
- if (isKnownNegation(I->getOperand(1), I->getOperand(2))) {
+ if (isKnownNegation(I->getOperand(1), I->getOperand(2), /*NeedNSW=*/false,
+ /*AllowPoison=*/false)) {
// Of one hand of select is known to be negation of another hand,
// just swap the hands around.
auto *NewSelect = cast<SelectInst>(I->clone());
diff --git a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
index f2a28c0dd02b39..b2e14ceaca1b08 100644
--- a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
+++ b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
@@ -1385,6 +1385,19 @@ define i8 @dont_negate_ordinary_select(i8 %x, i8 %y, i8 %z, i1 %c) {
ret i8 %t1
}
+define <2 x i32> @negate_select_of_negation_poison(<2 x i1> %c, <2 x i32> %x) {
+; CHECK-LABEL: @negate_select_of_negation_poison(
+; CHECK-NEXT: [[NEG:%.*]] = sub <2 x i32> <i32 0, i32 poison>, [[X:%.*]]
+; CHECK-NEXT: [[SEL:%.*]] = select <2 x i1> [[C:%.*]], <2 x i32> [[NEG]], <2 x i32> [[X]]
+; CHECK-NEXT: [[NEG2:%.*]] = sub <2 x i32> zeroinitializer, [[SEL]]
+; CHECK-NEXT: ret <2 x i32> [[NEG2]]
+;
+ %neg = sub <2 x i32> <i32 0, i32 poison>, %x
+ %sel = select <2 x i1> %c, <2 x i32> %neg, <2 x i32> %x
+ %neg2 = sub <2 x i32> zeroinitializer, %sel
+ ret <2 x i32> %neg2
+}
+
; Freeze is transparent as far as negation is concerned
define i4 @negate_freeze(i4 %x, i4 %y, i4 %z) {
; CHECK-LABEL: @negate_freeze(
|
Declining backport: This is an ABI breaking change, and while it would be possible to rewrite the fix in a way that does not break ABI, I don't think the fix itself is important to backport (this is not a regression, the issue exists since at least LLVM 12). |
Swapping the operands of a select is not valid if one hand is more poisonous that the other, because the negation zero contains poison elements.
Fix this by adding an extra parameter to isKnownNegation() to forbid poison elements.
I've implemented this using manual checks to avoid needing four variants for the NeedsNSW/AllowPoison combinations. Maybe there is a better way to do this...
Fixes #89669.
(cherry picked from commit a1b1c4a)