[ValueTracking] determine sign of 0.0 from select when matching min/max FP

[spatel]

Summary

In PR39475:
https://bugs.llvm.org/show_bug.cgi?id=39475
..we may fail to recognize/simplify fabs() in some cases because we do not canonicalize fcmp with a -0.0 operand.

Adding that canonicalization can cause regressions on min/max FP tests, so that's this patch: for the purpose of determining whether something is min/max, let the value returned by the select determine how we treat a 0.0 operand in the fcmp.

This patch doesn't actually change the -0.0 to +0.0. It just changes the analysis, so we don't fail to recognize equivalent min/max patterns that only differ in the signbit of 0.0.

[arsenm]

lib/Analysis/ValueTracking.cpp

4762–4773

This looks big. Can you avoid repeating the same thing for the true and false cases?

[spatel]

lib/Analysis/ValueTracking.cpp

4762–4773

Yes, it's a bit ugly. There really are 8 variations to test for from I see, but we can do better by matching with m_AnyZeroFP.
Unfortunately, I left out a potential vector mishap with undefs here, so I need to check for that.

[spatel]

Patch updated:

1. Reduced code by checking for any FP zero followed by mismatch.
2. Added checks and unit tests to ignore vector constants with undef elements.

[arsenm]

LGTM

lib/Analysis/ValueTracking.cpp

 }
 
 static SelectPatternResult matchSelectPattern(CmpInst::Predicate Pred,
                                               FastMathFlags FMF,
                                               Value *CmpLHS, Value *CmpRHS,
                                               Value *TrueVal, Value *FalseVal,
                                               Value *&LHS, Value *&RHS,
                                               unsigned Depth) {
+  if (CmpInst::isFPPredicate(Pred)) {
+    // If an FP compare has a 0.0 operand with a signbit that does not match
+    // a 0.0 operand of the select, then change the compare operand to match.
+    // This is allowed by IEEE-754: comparison ignores the sign of zero.
+    if (match(TrueVal, m_PosZeroFP()) && !match(FalseVal, m_NegZeroFP())) {
+      if (match(CmpLHS, m_NegZeroFP()))
+        CmpLHS = TrueVal;
+      if (match(CmpRHS, m_NegZeroFP()))
+        CmpRHS = TrueVal;
+    }
+    if (match(TrueVal, m_NegZeroFP()) && !match(FalseVal, m_PosZeroFP())) {
+      if (match(CmpLHS, m_PosZeroFP()))
+        CmpLHS = TrueVal;
+      if (match(CmpRHS, m_PosZeroFP()))
+        CmpRHS = TrueVal;
+    }
+    if (match(FalseVal, m_PosZeroFP()) && !match(TrueVal, m_NegZeroFP())) {
+      if (match(CmpLHS, m_NegZeroFP()))
+        CmpLHS = FalseVal;
+      if (match(CmpRHS, m_NegZeroFP()))
+        CmpRHS = FalseVal;
+    }
+    if (match(FalseVal, m_NegZeroFP()) && !match(TrueVal, m_PosZeroFP())) {
+      if (match(CmpLHS, m_PosZeroFP()))
+        CmpLHS = FalseVal;
+      if (match(CmpRHS, m_PosZeroFP()))
+        CmpRHS = FalseVal;
+    }
+  }
+
   LHS = CmpLHS;
   RHS = CmpRHS;
 
   // Signed zero may return inconsistent results between implementations.
   //  (0.0 <= -0.0) ? 0.0 : -0.0 // Returns 0.0
   //  minNum(0.0, -0.0)          // May return -0.0 or 0.0 (IEEE 754-2008 5.3.1)
   // Therefore, we behave conservatively and only proceed if at least one of the
   // operands is known to not be zero or if we don't care about signed zero.

test/Transforms/InstCombine/minmax-fp.ll

 ; CHECK-NEXT:    ret double [[TMP3]]
 ;
   %1 = fcmp ult float %a, 5.0
   %2 = fpext float %a to double
   %3 = select i1 %1, double %2, double 5.001
   ret double %3
 }
 
-; TODO:
-; From IEEE754: "Comparisons shall ignore the sign of zero (so +0 = −0)."
+; 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.
 
 define double @t6(float %a) {
 ; CHECK-LABEL: @t6(
-; 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]]
+; 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]]
 ;
   %1 = fcmp ult float %a, -0.0
   %2 = fpext float %a to double
   %3 = select i1 %1, double %2, double 0.0
   ret double %3
 }
 
-; TODO:
-; From IEEE754: "Comparisons shall ignore the sign of zero (so +0 = −0)."
+; 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.
 
 define double @t7(float %a) {
 ; CHECK-LABEL: @t7(
-; 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]]
+; 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]]
 ;
   %1 = fcmp ult float %a, 0.0
   %2 = fpext float %a to double
   %3 = select i1 %1, double %2, double -0.0
   ret double %3
 }
 
-; TODO:
 ; min(min(x, 0.0), 0.0) --> min(x, 0.0)
 
 define float @fmin_fmin_zero_mismatch(float %x) {
 ; CHECK-LABEL: @fmin_fmin_zero_mismatch(
-; CHECK-NEXT:    [[CMP1:%.*]] = fcmp olt float [[X:%.*]], -0.000000e+00
-; CHECK-NEXT:    [[MIN1:%.*]] = select i1 [[CMP1]], float [[X]], float 0.000000e+00
-; CHECK-NEXT:    [[CMP2:%.*]] = fcmp olt float [[MIN1]], 0.000000e+00
-; CHECK-NEXT:    [[MIN2:%.*]] = select i1 [[CMP2]], float [[MIN1]], float 0.000000e+00
+; CHECK-NEXT:    [[TMP1:%.*]] = fcmp olt float [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[MIN2:%.*]] = select i1 [[TMP1]], float [[X]], float 0.000000e+00
 ; CHECK-NEXT:    ret float [[MIN2]]
 ;
   %cmp1 = fcmp olt float %x, -0.0
   %min1 = select i1 %cmp1, float %x, float 0.0
   %cmp2 = fcmp olt float %min1, 0.0
   %min2 = select i1 %cmp2, float %min1, float 0.0
   ret float %min2
 }
 
-; TODO:
 ; 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:    [[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]]
+; 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]]
 ;
   %cmp1 = fcmp ogt float %x, 0.0
   %max1 = select i1 %cmp1, float %x, float -0.0
   %cmp2 = fcmp ogt float 0.0, %max1
   %max2 = select i1 %cmp2, float -0.0, float %max1
   ret float %max2
 }
 

unittests/Analysis/ValueTrackingTest.cpp

 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero1) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float -0.0, %a\n"
       "  %A = select i1 %1, float 0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero2) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float %a, -0.0\n"
       "  %A = select i1 %1, float 0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero3) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float 0.0, %a\n"
       "  %A = select i1 %1, float -0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero4) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float %a, 0.0\n"
       "  %A = select i1 %1, float -0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero5) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float -0.0, %a\n"
       "  %A = select i1 %1, float %a, float 0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero6) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float %a, -0.0\n"
       "  %A = select i1 %1, float %a, float 0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero7) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float 0.0, %a\n"
       "  %A = select i1 %1, float %a, float -0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMinMismatchConstantZero8) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float %a, 0.0\n"
       "  %A = select i1 %1, float %a, float -0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero1) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float -0.0, %a\n"
       "  %A = select i1 %1, float 0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero2) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float %a, -0.0\n"
       "  %A = select i1 %1, float 0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero3) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float 0.0, %a\n"
       "  %A = select i1 %1, float -0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero4) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float %a, 0.0\n"
       "  %A = select i1 %1, float -0.0, float %a\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_NAN, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero5) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float -0.0, %a\n"
       "  %A = select i1 %1, float %a, float 0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero6) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float %a, -0.0\n"
       "  %A = select i1 %1, float %a, float 0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, true});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero7) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp olt float 0.0, %a\n"
       "  %A = select i1 %1, float %a, float -0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, false});
 }
 
 TEST_F(MatchSelectPatternTest, FMaxMismatchConstantZero8) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float %a, 0.0\n"
       "  %A = select i1 %1, float %a, float -0.0\n"
       "  ret float %A\n"
       "}\n");
-  // FIXME: The sign of zero doesn't matter in fcmp.
-  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+  // The sign of zero doesn't matter in fcmp.
+  expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, true});
 }
 
 TEST_F(MatchSelectPatternTest, VectorFMinimum) {
   parseAssembly(
       "define <4 x float> @test(<4 x float> %a) {\n"
       "  %1 = fcmp ule <4 x float> %a, \n"
       "    <float 5.0, float 5.0, float 5.0, float 5.0>\n"
       "  %A = select <4 x i1> %1, <4 x float> %a,\n"