[InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum

[spatel]

Summary

This transform came up in D62414, but we should deal with it first.
We have LLVM intrinsics that correspond exactly to libm calls (unlike most libm calls, these libm calls never set errno).
This holds without any fast-math-flags, so we should always canonicalize to those intrinsics directly for better optimization.
Currently, we convert to fcmp+select only when we have FMF (nnan) because fcmp+select does not preserve the semantics of the call in the general case.

[cameron.mcinally]

LGTM

[spatel]

Patch updated:
Based on the discussion in D63294, we can't be too strict about matching libm calls in IR (a mismatched 'f' or 'l' suffix is ok), so I've updated the test that was checking for that case.

[spatel]

Ping.

[cameron.mcinally]

This LGTM at a high level, but I don't fully understand the AVR concerns from D63294. Maybe @eli.friedman would be a better reviewer?

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1583 ↗

(On Diff #205690)

It's pretty clear that the next IEEE-754 will respect zero sign bits for fmin/fmax. Would there be a big difference now if we didn't set nsz here? My thinking is that this line will become a bug when the new draft lands (and it's fairly hidden).

That said, if there is a behavior change by not setting the nsz flag, then we should probably just wait.

[spatel]

In D63214#1561143, @cameron.mcinally wrote:

This LGTM at a high level, but I don't fully understand the AVR concerns from D63294. Maybe @eli.friedman would be a better reviewer?

Yes - hoping that @efriedma will take a look when possible. As I understand it, FP types may be altered between C and LLVM, so what we think of as "float" or "double" is not necessarily consistent between C and LLVM. So I adjusted the test '@fake_fmin' to reflect that. I think that test is actually a fluke though because we are not consistently enforcing the mapping between lib function suffix and type (ie, we are probably doing other transforms using the less restrictive check that any FP type with a matching libm function name is a real libm call).

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1583 ↗

(On Diff #205690)

I'm fine with not including nsz here, but I think we should update the LangRef along with that change as a follow-up?

[efriedma]

LGTM

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

if (LdExpArg) {
 
       return CI;
     }
   }
   return Ret;
 }
 
 Value *LibCallSimplifier::optimizeFMinFMax(CallInst *CI, IRBuilder<> &B) {
-  Function *Callee = CI->getCalledFunction();
   // If we can shrink the call to a float function rather than a double
   // function, do that first.
+  Function *Callee = CI->getCalledFunction();
   StringRef Name = Callee->getName();
   if ((Name == "fmin" || Name == "fmax") && hasFloatVersion(Name))
     if (Value *Ret = optimizeBinaryDoubleFP(CI, B))
       return Ret;
 
-  IRBuilder<>::FastMathFlagGuard Guard(B);
-  FastMathFlags FMF;
-  if (CI->isFast()) {
-    // If the call is 'fast', then anything we create here will also be 'fast'.
-    FMF.setFast();
-  } else {
-    // At a minimum, no-nans-fp-math must be true.
-    if (!CI->hasNoNaNs())
-      return nullptr;
-    // No-signed-zeros is implied by the definitions of fmax/fmin themselves:
-    // "Ideally, fmax would be sensitive to the sign of zero, for example
-    // fmax(-0. 0, +0. 0) would return +0; however, implementation in software
-    // might be impractical."
-    FMF.setNoSignedZeros();
-    FMF.setNoNaNs();
-  }
+  // The LLVM intrinsics minnum/maxnum correspond to fmin/fmax. Canonicalize to
+  // the intrinsics for improved optimization (for example, vectorization).
+  // No-signed-zeros is implied by the definitions of fmax/fmin themselves.
+  // From the C standard draft WG14/N1256:
+  // fmax(-0.0, +0.0) would return +0; however, implementation in software
+  IRBuilder<>::FastMathFlagGuard Guard(B);
+  FastMathFlags FMF = CI->getFastMathFlags();
   B.setFastMathFlags(FMF);
 
-  // We have a relaxed floating-point environment. We can ignore NaN-handling
-  // and transform to a compare and select. We do not have to consider errno or
-  // exceptions, because fmin/fmax do not have those.
-  Value *Op0 = CI->getArgOperand(0);
-  Value *Op1 = CI->getArgOperand(1);
-  Value *Cmp = Callee->getName().startswith("fmin") ?
-    B.CreateFCmpOLT(Op0, Op1) : B.CreateFCmpOGT(Op0, Op1);
-  return B.CreateSelect(Cmp, Op0, Op1);
+  Intrinsic::ID IID = Callee->getName().startswith("fmin") ? Intrinsic::minnum
+                                                           : Intrinsic::maxnum;
+  Function *F = Intrinsic::getDeclaration(CI->getModule(), IID, CI->getType());
+  return B.CreateCall(F, { CI->getArgOperand(0), CI->getArgOperand(1) });
 }
 
 Value *LibCallSimplifier::optimizeLog(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
   Value *Ret = nullptr;
   StringRef Name = Callee->getName();
   if (UnsafeFPShrink && hasFloatVersion(Name))
     Ret = optimizeUnaryDoubleFP(CI, B, true);

llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll

;
   %call = call fast double @tanh(double %conv)
   ret double %call
 }
 
 ; 'arcp' on an fmax() is meaningless. This test just proves that
 ; flags are propagated for shrunken *binary* double FP calls.
 define float @max1(float %a, float %b) {
 ; CHECK-LABEL: @max1(
-; ISC99-NEXT:    [[FMAXF:%.*]] = call arcp float @fmaxf(float [[A:%.*]], float [[B:%.*]])
+; ISC99-NEXT:    [[FMAXF:%.*]] = call nsz arcp float @llvm.maxnum.f32(float [[A:%.*]], float [[B:%.*]])
 ; ISC99-NEXT:    ret float [[FMAXF]]
 ; ISC89:         [[FMAXF:%.*]] = call arcp double @fmax(double [[A:%.*]], double [[B:%.*]])
 ;
   %c = fpext float %a to double
   %d = fpext float %b to double
   %e = call arcp double @fmax(double %c, double %d)
   %f = fptrunc double %e to float
   ret float %f
 }
 
-; A function can have a name that matches a common libcall,
-; but with the wrong type(s). Let it be.
+; This is treated as libm 'fmin' - LLVM types do not necessarily
+; correspond to 'C' types, so this is not required to be "fminl".
 
 define float @fake_fmin(float %a, float %b) {
 ; CHECK-LABEL: @fake_fmin(
 ; CHECK-NEXT:    [[C:%.*]] = fpext float [[A:%.*]] to fp128
 ; CHECK-NEXT:    [[D:%.*]] = fpext float [[B:%.*]] to fp128
-; CHECK-NEXT:    [[E:%.*]] = call fp128 @fmin(fp128 [[C]], fp128 [[D]])
+; ISC99-NEXT:    [[E:%.*]] = call nsz fp128 @llvm.minnum.f128(fp128 [[C]], fp128 [[D]])
+; ISC89-NEXT:    [[E:%.*]] = call fp128 @fmin(fp128 [[C]], fp128 [[D]])
 ; CHECK-NEXT:    [[F:%.*]] = fptrunc fp128 [[E]] to float
 ; CHECK-NEXT:    ret float [[F]]
 ;
   %c = fpext float %a to fp128
   %d = fpext float %b to fp128
   %e = call fp128 @fmin(fp128 %c, fp128 %d)
   %f = fptrunc fp128 %e to float
   ret float %f
 }
 
-declare fp128 @fmin(fp128, fp128) ; This is not the 'fmin' you're looking for.
+declare fp128 @fmin(fp128, fp128)
 
 declare double @fmax(double, double)
 
 declare double @tanh(double)
 declare double @tan(double)
 
 ; sqrt is a special case: the shrinking optimization
 ; is valid even without unsafe-fp-math.

llvm/trunk/test/Transforms/InstCombine/fast-math.ll

 
 declare double @fmax(double, double)
 declare double @fmin(double, double)
 declare float @fmaxf(float, float)
 declare float @fminf(float, float)
 declare fp128 @fmaxl(fp128, fp128)
 declare fp128 @fminl(fp128, fp128)
 
-; No NaNs is the minimum requirement to replace these calls.
-; This should always be set when unsafe-fp-math is true, but
-; alternate the attributes for additional test coverage.
 ; 'nsz' is implied by the definition of fmax or fmin itself.
 
-; Shrink and remove the call.
+; Shrink and replace the call.
 define float @max1(float %a, float %b) {
 ; CHECK-LABEL: @max1(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp fast ogt float [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select fast i1 [[TMP1]], float [[A]], float [[B]]
-; CHECK-NEXT:    ret float [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast float @llvm.maxnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = fpext float %a to double
   %d = fpext float %b to double
   %e = call fast double @fmax(double %c, double %d)
   %f = fptrunc double %e to float
   ret float %f
 }
 
 define float @fmax_no_fmf(float %a, float %b) {
 ; CHECK-LABEL: @fmax_no_fmf(
-; CHECK-NEXT:    [[C:%.*]] = call float @fmaxf(float [[A:%.*]], float [[B:%.*]])
-; CHECK-NEXT:    ret float [[C]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.maxnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = call float @fmaxf(float %a, float %b)
   ret float %c
 }
 
 define float @max2(float %a, float %b) {
 ; CHECK-LABEL: @max2(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp nnan nsz ogt float [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select nnan nsz i1 [[TMP1]], float [[A]], float [[B]]
-; CHECK-NEXT:    ret float [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz float @llvm.maxnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = call nnan float @fmaxf(float %a, float %b)
   ret float %c
 }
 
 
 define double @max3(double %a, double %b) {
 ; CHECK-LABEL: @max3(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp fast ogt double [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select fast i1 [[TMP1]], double [[A]], double [[B]]
-; CHECK-NEXT:    ret double [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast double @llvm.maxnum.f64(double [[A:%.*]], double [[B:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %c = call fast double @fmax(double %a, double %b)
   ret double %c
 }
 
 define fp128 @max4(fp128 %a, fp128 %b) {
 ; CHECK-LABEL: @max4(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp nnan nsz ogt fp128 [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select nnan nsz i1 [[TMP1]], fp128 [[A]], fp128 [[B]]
-; CHECK-NEXT:    ret fp128 [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz fp128 @llvm.maxnum.f128(fp128 [[A:%.*]], fp128 [[B:%.*]])
+; CHECK-NEXT:    ret fp128 [[TMP1]]
 ;
   %c = call nnan fp128 @fmaxl(fp128 %a, fp128 %b)
   ret fp128 %c
 }
 
 ; Shrink and remove the call.
 define float @min1(float %a, float %b) {
 ; CHECK-LABEL: @min1(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp nnan nsz olt float [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select nnan nsz i1 [[TMP1]], float [[A]], float [[B]]
-; CHECK-NEXT:    ret float [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz float @llvm.minnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = fpext float %a to double
   %d = fpext float %b to double
   %e = call nnan double @fmin(double %c, double %d)
   %f = fptrunc double %e to float
   ret float %f
 }
 
 define float @fmin_no_fmf(float %a, float %b) {
 ; CHECK-LABEL: @fmin_no_fmf(
-; CHECK-NEXT:    [[C:%.*]] = call float @fminf(float [[A:%.*]], float [[B:%.*]])
-; CHECK-NEXT:    ret float [[C]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.minnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = call float @fminf(float %a, float %b)
   ret float %c
 }
 
 define float @min2(float %a, float %b) {
 ; CHECK-LABEL: @min2(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp fast olt float [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select fast i1 [[TMP1]], float [[A]], float [[B]]
-; CHECK-NEXT:    ret float [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast float @llvm.minnum.f32(float [[A:%.*]], float [[B:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %c = call fast float @fminf(float %a, float %b)
   ret float %c
 }
 
 define double @min3(double %a, double %b) {
 ; CHECK-LABEL: @min3(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp nnan nsz olt double [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select nnan nsz i1 [[TMP1]], double [[A]], double [[B]]
-; CHECK-NEXT:    ret double [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.minnum.f64(double [[A:%.*]], double [[B:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %c = call nnan double @fmin(double %a, double %b)
   ret double %c
 }
 
 define fp128 @min4(fp128 %a, fp128 %b) {
 ; CHECK-LABEL: @min4(
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp fast olt fp128 [[A:%.*]], [[B:%.*]]
-; CHECK-NEXT:    [[TMP2:%.*]] = select fast i1 [[TMP1]], fp128 [[A]], fp128 [[B]]
-; CHECK-NEXT:    ret fp128 [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast fp128 @llvm.minnum.f128(fp128 [[A:%.*]], fp128 [[B:%.*]])
+; CHECK-NEXT:    ret fp128 [[TMP1]]
 ;
   %c = call fast fp128 @fminl(fp128 %a, fp128 %b)
   ret fp128 %c
 }
 
 ; ((which ? 2.0 : a) + 1.0) => (which ? 3.0 : (a + 1.0))
 ; This is always safe.  No FMF required.
 define float @test55(i1 %which, float %a) {

llvm/trunk/test/Transforms/InstCombine/float-shrink-compare.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -instcombine < %s | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 
 define i1 @test1(float %x, float %y) {
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:    [[CEIL:%.*]] = call float @llvm.ceil.f32(float %x)
 ; CHECK-NEXT:    [[CMP:%.*]] = fcmp oeq float [[CEIL]], %y
;
   %y.ext = fpext float %y to double
   %trunc = call double @llvm.trunc.f64(double %x.ext) nounwind
   %cmp = fcmp oeq double %y.ext, %trunc
   ret i1 %cmp
 }
 
 define i1 @test15(float %x, float %y, float %z) {
 ; CHECK-LABEL: @test15(
-; CHECK-NEXT:    [[FMINF:%.*]] = call float @fminf(float %x, float %y) #0
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq float [[FMINF]], %z
-; CHECK-NEXT:    ret i1 [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.minnum.f32(float [[X:%.*]], float [[Y:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = fcmp oeq float [[TMP1]], [[Z:%.*]]
+; CHECK-NEXT:    ret i1 [[TMP2]]
 ;
   %1 = fpext float %x to double
   %2 = fpext float %y to double
   %3 = call double @fmin(double %1, double %2) nounwind
   %4 = fpext float %z to double
   %5 = fcmp oeq double %3, %4
   ret i1 %5
 }
 
 define i1 @test16(float %x, float %y, float %z) {
 ; CHECK-LABEL: @test16(
-; CHECK-NEXT:    [[FMINF:%.*]] = call float @fminf(float %x, float %y) #0
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq float [[FMINF]], %z
-; CHECK-NEXT:    ret i1 [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.minnum.f32(float [[X:%.*]], float [[Y:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = fcmp oeq float [[TMP1]], [[Z:%.*]]
+; CHECK-NEXT:    ret i1 [[TMP2]]
 ;
   %1 = fpext float %z to double
   %2 = fpext float %x to double
   %3 = fpext float %y to double
   %4 = call double @fmin(double %2, double %3) nounwind
   %5 = fcmp oeq double %1, %4
   ret i1 %5
 }
 
 define i1 @test17(float %x, float %y, float %z) {
 ; CHECK-LABEL: @test17(
-; CHECK-NEXT:    [[FMAXF:%.*]] = call float @fmaxf(float %x, float %y) #0
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq float [[FMAXF]], %z
-; CHECK-NEXT:    ret i1 [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.maxnum.f32(float [[X:%.*]], float [[Y:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = fcmp oeq float [[TMP1]], [[Z:%.*]]
+; CHECK-NEXT:    ret i1 [[TMP2]]
 ;
   %1 = fpext float %x to double
   %2 = fpext float %y to double
   %3 = call double @fmax(double %1, double %2) nounwind
   %4 = fpext float %z to double
   %5 = fcmp oeq double %3, %4
   ret i1 %5
 }
 
 define i1 @test18(float %x, float %y, float %z) {
 ; CHECK-LABEL: @test18(
-; CHECK-NEXT:    [[FMAXF:%.*]] = call float @fmaxf(float %x, float %y) #0
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq float [[FMAXF]], %z
-; CHECK-NEXT:    ret i1 [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.maxnum.f32(float [[X:%.*]], float [[Y:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = fcmp oeq float [[TMP1]], [[Z:%.*]]
+; CHECK-NEXT:    ret i1 [[TMP2]]
 ;
   %1 = fpext float %z to double
   %2 = fpext float %x to double
   %3 = fpext float %y to double
   %4 = call double @fmax(double %2, double %3) nounwind
   %5 = fcmp oeq double %1, %4
   ret i1 %5
 }
;
   %3 = call double @copysign(double %1, double %2) nounwind
   %4 = fpext float %z to double
   %5 = fcmp oeq double %3, %4
   ret i1 %5
 }
 
 define i1 @test20(float %x, float %y) {
 ; CHECK-LABEL: @test20(
-; CHECK-NEXT:    [[FMINF:%.*]] = call float @fminf(float 1.000000e+00, float %x) #0
-; CHECK-NEXT:    [[TMP1:%.*]] = fcmp oeq float [[FMINF]], %y
-; CHECK-NEXT:    ret i1 [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.minnum.f32(float [[X:%.*]], float 1.000000e+00)
+; CHECK-NEXT:    [[TMP2:%.*]] = fcmp oeq float [[TMP1]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[TMP2]]
 ;
   %1 = fpext float %y to double
   %2 = fpext float %x to double
   %3 = call double @fmin(double 1.000000e+00, double %2) nounwind
   %4 = fcmp oeq double %1, %3
   ret i1 %4
 }
 
 ; should not be changed to fminf as the constant would lose precision
 
 define i1 @test21(float %x, float %y) {
 ; CHECK-LABEL: @test21(
-; CHECK-NEXT:    [[TMP1:%.*]] = fpext float %y to double
-; CHECK-NEXT:    [[TMP2:%.*]] = fpext float %x to double
-; CHECK-NEXT:    [[TMP3:%.*]] = call double @fmin(double 1.300000e+00, double [[TMP2]]) #2
+; CHECK-NEXT:    [[TMP1:%.*]] = fpext float [[Y:%.*]] to double
+; CHECK-NEXT:    [[TMP2:%.*]] = fpext float [[X:%.*]] to double
+; CHECK-NEXT:    [[TMP3:%.*]] = call nsz double @llvm.minnum.f64(double [[TMP2]], double 1.300000e+00)
 ; CHECK-NEXT:    [[TMP4:%.*]] = fcmp oeq double [[TMP3]], [[TMP1]]
 ; CHECK-NEXT:    ret i1 [[TMP4]]
 ;
   %1 = fpext float %y to double
   %2 = fpext float %x to double
   %3 = call double @fmin(double 1.300000e+00, double %2) nounwind
   %4 = fcmp oeq double %1, %3
   ret i1 %4