[llvm] [InstCombine] Fix miscompile in negation of select (PR #89698)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 23 17:35:01 PDT 2024
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/89698
>From 51967fe7e8e9668f3076b7db812193c1b2487c51 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Tue, 23 Apr 2024 10:37:58 +0900
Subject: [PATCH 1/2] [InstCombine] Fix miscompile in negation of select
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 https://github.com/llvm/llvm-project/issues/89669.
---
llvm/include/llvm/Analysis/ValueTracking.h | 3 ++-
llvm/lib/Analysis/ValueTracking.cpp | 23 ++++++++++++++-----
.../InstCombine/InstCombineNegator.cpp | 3 ++-
.../InstCombine/sub-of-negatible.ll | 6 ++---
4 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index a2fa8f6064e116..571e44cdac2650 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -131,7 +131,8 @@ bool isKnownNonZero(const Value *V, const SimplifyQuery &Q, 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 21e3f8a4cc52c7..36d9010e34ba14 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -8042,17 +8042,28 @@ 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_NSWNeg(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;
+ };
+ // X = sub (0, Y) || X = sub nsw (0, Y)
// Y = sub (0, X) || Y = sub nsw (0, X)
- if ((!NeedNSW && match(Y, m_Sub(m_ZeroInt(), m_Specific(X)))) ||
- (NeedNSW && match(Y, m_NSWNeg(m_Specific(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 d697f361dec023..ed2a98ba4ae40e 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 72fd7f7be2b04c..b2e14ceaca1b08 100644
--- a/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
+++ b/llvm/test/Transforms/InstCombine/sub-of-negatible.ll
@@ -1385,12 +1385,12 @@ define i8 @dont_negate_ordinary_select(i8 %x, i8 %y, i8 %z, i1 %c) {
ret i8 %t1
}
-; FIXME: This is a miscompile.
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: [[TMP1:%.*]] = select <2 x i1> [[C:%.*]], <2 x i32> [[X]], <2 x i32> [[NEG]]
-; CHECK-NEXT: ret <2 x i32> [[TMP1]]
+; 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
>From 61413cf0b92ffcd828dd34a1f7bd3e48440fc6d8 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 24 Apr 2024 09:34:35 +0900
Subject: [PATCH 2/2] Simplify comment
---
llvm/lib/Analysis/ValueTracking.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 36d9010e34ba14..7501f78ca23b7b 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -8061,8 +8061,7 @@ bool llvm::isKnownNegation(const Value *X, const Value *Y, bool NeedNSW,
return true;
};
- // X = sub (0, Y) || X = sub nsw (0, Y)
- // Y = sub (0, X) || Y = sub nsw (0, X)
+ // X = -Y or Y = -X
if (IsNegationOf(X, Y) || IsNegationOf(Y, X))
return true;
More information about the llvm-commits
mailing list