[llvm] 4e44c22 - [ValueTracking][InstCombine] restrict FP min/max matching to avoid miscompile
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 25 13:52:49 PDT 2022
Author: Sanjay Patel
Date: 2022-08-25T16:52:40-04:00
New Revision: 4e44c22c97be99f7c3864fb9fc68b3295835d837
URL: https://github.com/llvm/llvm-project/commit/4e44c22c97be99f7c3864fb9fc68b3295835d837
DIFF: https://github.com/llvm/llvm-project/commit/4e44c22c97be99f7c3864fb9fc68b3295835d837.diff
LOG: [ValueTracking][InstCombine] restrict FP min/max matching to avoid miscompile
This is a long-standing FIXME with a non-FMF test that exposes
the bug as shown in issue #57357.
It's possible that there's still a way to miscompile by
mis-identifying/mis-folding FP min/max patterns, but
this patch only exposes a couple of seemingly minor
regressions while preventing the broken transform.
Added:
Modified:
llvm/lib/Analysis/ValueTracking.cpp
llvm/test/Transforms/InstCombine/minmax-fp.ll
llvm/unittests/Analysis/ValueTrackingTest.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index b2393de81fc4e..5f22714f40890 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6085,6 +6085,7 @@ static SelectPatternResult matchSelectPattern(CmpInst::Predicate Pred,
Value *TrueVal, Value *FalseVal,
Value *&LHS, Value *&RHS,
unsigned Depth) {
+ bool HasMismatchedZeros = false;
if (CmpInst::isFPPredicate(Pred)) {
// IEEE-754 ignores the sign of 0.0 in comparisons. So if the select has one
// 0.0 operand, set the compare's 0.0 operands to that same value for the
@@ -6099,10 +6100,14 @@ static SelectPatternResult matchSelectPattern(CmpInst::Predicate Pred,
OutputZeroVal = FalseVal;
if (OutputZeroVal) {
- if (match(CmpLHS, m_AnyZeroFP()))
+ if (match(CmpLHS, m_AnyZeroFP()) && CmpLHS != OutputZeroVal) {
+ HasMismatchedZeros = true;
CmpLHS = OutputZeroVal;
- if (match(CmpRHS, m_AnyZeroFP()))
+ }
+ if (match(CmpRHS, m_AnyZeroFP()) && CmpRHS != OutputZeroVal) {
+ HasMismatchedZeros = true;
CmpRHS = OutputZeroVal;
+ }
}
}
@@ -6116,7 +6121,11 @@ static SelectPatternResult matchSelectPattern(CmpInst::Predicate Pred,
// operands is known to not be zero or if we don't care about signed zero.
switch (Pred) {
default: break;
- // FIXME: Include OGT/OLT/UGT/ULT.
+ case CmpInst::FCMP_OGT: case CmpInst::FCMP_OLT:
+ case CmpInst::FCMP_UGT: case CmpInst::FCMP_ULT:
+ if (!HasMismatchedZeros)
+ break;
+ [[fallthrough]];
case CmpInst::FCMP_OGE: case CmpInst::FCMP_OLE:
case CmpInst::FCMP_UGE: case CmpInst::FCMP_ULE:
if (!FMF.noSignedZeros() && !isKnownNonZero(CmpLHS) &&
diff --git a/llvm/test/Transforms/InstCombine/minmax-fp.ll b/llvm/test/Transforms/InstCombine/minmax-fp.ll
index e687c8f073a6e..edb0f341c8152 100644
--- a/llvm/test/Transforms/InstCombine/minmax-fp.ll
+++ b/llvm/test/Transforms/InstCombine/minmax-fp.ll
@@ -57,13 +57,13 @@ define double @t5(float %a) {
ret double %3
}
-; FIXME: This is a miscompile. PR57357
+; PR57357
define float @not_maxnum(float %x) {
; CHECK-LABEL: @not_maxnum(
-; CHECK-NEXT: [[DOTINV:%.*]] = fcmp ole float [[X:%.*]], 0.000000e+00
-; CHECK-NEXT: [[TMP1:%.*]] = select i1 [[DOTINV]], float -0.000000e+00, float [[X]]
-; CHECK-NEXT: ret float [[TMP1]]
+; CHECK-NEXT: [[CMP:%.*]] = fcmp olt float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], float -0.000000e+00, float [[X]]
+; CHECK-NEXT: ret float [[SEL]]
;
%cmp = fcmp olt float %x, 0.0
%sel = select i1 %cmp, float -0.0, float %x
@@ -86,15 +86,15 @@ define double @t6(float %a) {
ret double %3
}
-; From IEEE754: "Comparisons shall ignore the sign of zero (so +0 = -0)."
-; So the compare constant may be treated as -0.0, and we sink the fpext.
+; TODO: Move the select ahead of the fpext because it's a narrower op.
+; This works as long as the wide constant can be represented exactly in the narrow type.
define double @t7(float %a) {
; CHECK-LABEL: @t7(
-; CHECK-NEXT: [[DOTINV:%.*]] = fcmp oge float [[A:%.*]], 0.000000e+00
-; CHECK-NEXT: [[TMP1:%.*]] = select i1 [[DOTINV]], float -0.000000e+00, float [[A]]
-; CHECK-NEXT: [[TMP2:%.*]] = fpext float [[TMP1]] to double
-; CHECK-NEXT: ret double [[TMP2]]
+; CHECK-NEXT: [[TMP1:%.*]] = fcmp ult float [[A:%.*]], 0.000000e+00
+; CHECK-NEXT: [[TMP2:%.*]] = fpext float [[A]] to double
+; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP1]], double [[TMP2]], double -0.000000e+00
+; CHECK-NEXT: ret double [[TMP3]]
;
%1 = fcmp ult float %a, 0.0
%2 = fpext float %a to double
@@ -106,8 +106,8 @@ define double @t7(float %a) {
define float @fmin_fmin_zero_mismatch(float %x) {
; CHECK-LABEL: @fmin_fmin_zero_mismatch(
-; CHECK-NEXT: [[TMP1:%.*]] = fcmp olt float [[X:%.*]], 0.000000e+00
-; CHECK-NEXT: [[MIN2:%.*]] = select i1 [[TMP1]], float [[X]], float 0.000000e+00
+; CHECK-NEXT: [[CMP1:%.*]] = fcmp olt float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT: [[MIN2:%.*]] = select i1 [[CMP1]], float [[X]], float 0.000000e+00
; CHECK-NEXT: ret float [[MIN2]]
;
%cmp1 = fcmp olt float %x, -0.0
@@ -117,13 +117,16 @@ define float @fmin_fmin_zero_mismatch(float %x) {
ret float %min2
}
+; TODO: We do not recognize these as max ops because of the mismatched zeros.
; max(max(x, -0.0), -0.0) --> max(x, -0.0)
define float @fmax_fmax_zero_mismatch(float %x) {
; CHECK-LABEL: @fmax_fmax_zero_mismatch(
-; CHECK-NEXT: [[TMP1:%.*]] = fcmp ogt float [[X:%.*]], -0.000000e+00
-; CHECK-NEXT: [[MAX11:%.*]] = select i1 [[TMP1]], float [[X]], float -0.000000e+00
-; CHECK-NEXT: ret float [[MAX11]]
+; CHECK-NEXT: [[CMP1:%.*]] = fcmp ogt float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT: [[MAX1:%.*]] = select i1 [[CMP1]], float [[X]], float -0.000000e+00
+; CHECK-NEXT: [[CMP2:%.*]] = fcmp olt float [[MAX1]], 0.000000e+00
+; CHECK-NEXT: [[MAX2:%.*]] = select i1 [[CMP2]], float -0.000000e+00, float [[MAX1]]
+; CHECK-NEXT: ret float [[MAX2]]
;
%cmp1 = fcmp ogt float %x, 0.0
%max1 = select i1 %cmp1, float %x, float -0.0
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 902107d30082c..05e6a099c3a5b 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -202,7 +202,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero1) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero2) {
@@ -213,7 +213,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero2) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero3) {
@@ -224,7 +224,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero3) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero4) {
@@ -235,7 +235,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero4) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero5) {
@@ -246,7 +246,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero5) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero6) {
@@ -257,7 +257,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero6) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero7) {
@@ -268,7 +268,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero7) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero8) {
@@ -279,7 +279,7 @@ TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero8) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero1) {
@@ -290,7 +290,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero1) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero2) {
@@ -301,7 +301,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero2) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero3) {
@@ -312,7 +312,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero3) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero4) {
@@ -323,7 +323,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero4) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero5) {
@@ -334,7 +334,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero5) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero6) {
@@ -345,7 +345,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero6) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero7) {
@@ -356,7 +356,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero7) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, false});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero8) {
@@ -367,7 +367,7 @@ TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero8) {
" ret float %A\n"
"}\n");
// The sign of zero doesn't matter in fcmp.
- expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, true});
+ expectPattern({SPF_UNKNOWN, SPNB_NA, false});
}
TEST_F(MatchSelectPatternTest, FMinMismatchConstantZeroVecUndef) {
More information about the llvm-commits
mailing list