Fix the use of -fno-approx-func along with -Ofast or -ffast-math

[masoud.ataei]

Summary

Fixing the bug number Bug 52565: https://bugs.llvm.org/show_bug.cgi?id=52565

[andrew.w.kaylor]

Thanks for the patch! This looks mostly good. I have just a few suggestions.

Could you add test cases in clang/test/Driver/clang_f_opts.c to verify that the various driver inputs get overridden in the expected way? Without such a test, this behavior is likely to be broken in the future.

clang/lib/Driver/ToolChains/Clang.cpp

2760

Should this also imply "MathErrno = false"?

2762

Should this conflict with -fapprox-func?

2880

I think you need "AppoxFunc = false" here.

2938

You need a check for "!ApproxFunc" here.

[masoud.ataei]

Updated the test combining -ffast-math and -fno-approx-func options.
@andrew.w.kaylor I hope you don't mind that I put those tests on clang/test/Driver/fast-math.c instead.

[masoud.ataei]

clang/lib/Driver/ToolChains/Clang.cpp

2760

I don't think setting ApproxFunc to true should imply "MathErrno = false".

Let say someone have a math library that compute approximate result for none special input/output but returns NaN, INF and errno correctly otherwise. That is actually can be fairly common, because performance in the none special cases are much more important that the special ones. So returning errno in the special outputs theoretically should not effect the performance on the main path. Therefore, I think compiler should not assume anything about MathErrno value based on ApproxFunc value.

2762

Same as above.

[masoud.ataei]

Gentle reminder for reviewers. -- This PR is ready for review.

[aaron.ballman]

Removing myself as a reviewer because this is just far enough outside my expertise to feel uncomfortable reviewing it, but added a few trusted folks who have expressed opinions in this area before.

[masoud.ataei]

clang/lib/Driver/ToolChains/Clang.cpp

2760

I am not sure what I was suggesting in my last comment is correct or not. Can one of the more experienced reviewers confirm?
The question is: Should "ApproxFunc=ture" implies "MathErrno=false"?

[joerg]

clang/lib/Driver/ToolChains/Clang.cpp

2760

They seem pretty orthogonal to me.

[masoud.ataei]

A gentle reminder for reviewers. -- This patch will fix the bug reported here: https://bugs.llvm.org/show_bug.cgi?id=52565

[andrew.w.kaylor]

lgtm

clang/lib/Driver/ToolChains/Clang.cpp

2760

I see the point of your explanation. I'm satisfied that they should remain separate.

clang/lib/Driver/ToolChains/Clang.cpp

for (const Arg *A : Args) {
     // If this isn't an FP option skip the claim below
     default: continue;
 
     // Options controlling individual features
     case options::OPT_fhonor_infinities:    HonorINFs = true;         break;
     case options::OPT_fno_honor_infinities: HonorINFs = false;        break;
     case options::OPT_fhonor_nans:          HonorNaNs = true;         break;
     case options::OPT_fno_honor_nans:       HonorNaNs = false;        break;
     case options::OPT_fapprox_func:         ApproxFunc = true;        break;
     case options::OPT_fno_approx_func:      ApproxFunc = false;       break;
     case options::OPT_fmath_errno:          MathErrno = true;         break;
     case options::OPT_fno_math_errno:       MathErrno = false;        break;
     case options::OPT_fassociative_math:    AssociativeMath = true;   break;
     case options::OPT_fno_associative_math: AssociativeMath = false;  break;
     case options::OPT_freciprocal_math:     ReciprocalMath = true;    break;
     case options::OPT_fno_reciprocal_math:  ReciprocalMath = false;   break;
     case options::OPT_fsigned_zeros:        SignedZeros = true;       break;
     case options::OPT_fno_signed_zeros:     SignedZeros = false;      break;
     case options::OPT_ftrapping_math:
case options::OPT_fno_finite_math_only:
       HonorINFs = true;
       HonorNaNs = true;
       break;
 
     case options::OPT_funsafe_math_optimizations:
       AssociativeMath = true;
       ReciprocalMath = true;
       SignedZeros = false;
+      ApproxFunc = true;
       TrappingMath = false;
       FPExceptionBehavior = "";
       break;
     case options::OPT_fno_unsafe_math_optimizations:
       AssociativeMath = false;
       ReciprocalMath = false;
       SignedZeros = true;
+      ApproxFunc = false;
       TrappingMath = true;
       FPExceptionBehavior = "strict";
 
       // The target may have opted to flush by default, so force IEEE.
       DenormalFPMath = llvm::DenormalMode::getIEEE();
       DenormalFP32Math = llvm::DenormalMode::getIEEE();
       break;
 
     case options::OPT_Ofast:
       // If -Ofast is the optimization level, then -ffast-math should be enabled
       if (!OFastEnabled)
         continue;
       LLVM_FALLTHROUGH;
     case options::OPT_ffast_math:
       HonorINFs = false;
       HonorNaNs = false;
       MathErrno = false;
       AssociativeMath = true;
       ReciprocalMath = true;
+      ApproxFunc = true;
       SignedZeros = false;
       TrappingMath = false;
       RoundingFPMath = false;
       // If fast-math is set then set the fp-contract mode to fast.
       FPContract = "fast";
       break;
     case options::OPT_fno_fast_math:
       HonorINFs = true;
       HonorNaNs = true;
       // Turning on -ffast-math (with either flag) removes the need for
       // MathErrno. However, turning *off* -ffast-math merely restores the
       // toolchain default (which may be false).
       MathErrno = TC.IsMathErrnoDefault();
       AssociativeMath = false;
       ReciprocalMath = false;
+      ApproxFunc = false;
       SignedZeros = true;
       // -fno_fast_math restores default denormal and fpcontract handling
       DenormalFPMath = DefaultDenormalFPMath;
       DenormalFP32Math = llvm::DenormalMode::getIEEE();
       if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
           !JA.isOffloading(Action::OFK_HIP))
         if (FPContract == "fast") {
           FPContract = "on";
           D.Diag(clang::diag::warn_drv_overriding_flag_option)
               << "-ffp-contract=fast"
               << "-ffp-contract=on";
         }
       break;
     }
     if (StrictFPModel) {
       // If -ffp-model=strict has been specified on command line but
       // subsequent options conflict then emit warning diagnostic.
       if (HonorINFs && HonorNaNs && !AssociativeMath && !ReciprocalMath &&
-          SignedZeros && TrappingMath && RoundingFPMath &&
+          SignedZeros && TrappingMath && RoundingFPMath && !ApproxFunc &&
           DenormalFPMath == llvm::DenormalMode::getIEEE() &&
           DenormalFP32Math == llvm::DenormalMode::getIEEE() &&
           FPContract.equals("off"))
         // OK: Current Arg doesn't conflict with -ffp-model=strict
         ;
       else {
         StrictFPModel = false;
         FPModel = "";
static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
 
   if (ApproxFunc)
     CmdArgs.push_back("-fapprox-func");
 
   if (MathErrno)
     CmdArgs.push_back("-fmath-errno");
 
   if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
-      !TrappingMath)
+      ApproxFunc && !TrappingMath)
     CmdArgs.push_back("-menable-unsafe-fp-math");
 
   if (!SignedZeros)
     CmdArgs.push_back("-fno-signed-zeros");
 
   if (AssociativeMath && !SignedZeros && !TrappingMath)
     CmdArgs.push_back("-mreassociate");
 
if (!FPExceptionBehavior.empty())
     CmdArgs.push_back(Args.MakeArgString("-ffp-exception-behavior=" +
                       FPExceptionBehavior));
 
   ParseMRecip(D, Args, CmdArgs);
 
   // -ffast-math enables the __FAST_MATH__ preprocessor macro, but check for the
   // individual features enabled by -ffast-math instead of the option itself as
   // that's consistent with gcc's behaviour.
-  if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
+  if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath && ApproxFunc &&
       ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
     CmdArgs.push_back("-ffast-math");
     if (FPModel.equals("fast")) {
       if (FPContract.equals("fast"))
         // All set, do nothing.
         ;
       else if (FPContract.empty())
         // Enable -ffp-contract=fast

clang/test/Driver/fast-math.c

 // CHECK-NO-FAST-MATH-NO-NANS: "-cc1"
 // CHECK-NO-FAST-MATH-NO-NANS: "-menable-no-nans"
 //
 // RUN: %clang -### -fno-honor-nans -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NANS-NO-FAST-MATH %s
 // CHECK-NO-NANS-NO-FAST-MATH: "-cc1"
 // CHECK-NO-NANS-NO-FAST-MATH-NOT: "-menable-no-nans"
 //
+// RUN: %clang -### -ffast-math -fno-approx-func -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH-NO-APPROX-FUNC %s
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-cc1"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-menable-no-infs"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-menable-no-nans"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-fno-signed-zeros"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-mreassociate"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-freciprocal-math"
+// CHECK-FAST-MATH-NO-APPROX-FUNC:     "-ffp-contract=fast"
+// CHECK-FAST-MATH-NO-APPROX-FUNC-NOT: "-ffast-math"
+// CHECK-FAST-MATH-NO-APPROX-FUNC-NOT: "-fapprox-func"
+//
+// RUN: %clang -### -fno-approx-func -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-APPROX-FUNC-FAST-MATH %s
+// CHECK-NO-APPROX-FUNC-FAST-MATH: "-cc1"
+// CHECK-NO-APPROX-FUNC-FAST-MATH: "-ffast-math"
+//
 // RUN: %clang -### -fapprox-func -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-APPROX-FUNC %s
 // CHECK-APPROX-FUNC: "-cc1"
 // CHECK-APPROX-FUNC: "-fapprox-func"
 //
 // RUN: %clang -### -fmath-errno -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MATH-ERRNO %s
 // CHECK-MATH-ERRNO: "-cc1"

 // RUN: %clang -### -target i686-apple-darwin -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-MATH-ERRNO %s
 // RUN: %clang -### -target i686-apple-darwin -fno-math-errno -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-MATH-ERRNO %s
 // RUN: %clang -### -fno-fast-math -fno-math-errno -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-MATH-ERRNO %s
 //
 // RUN: %clang -### -fno-math-errno -fassociative-math -freciprocal-math \
-// RUN:     -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \
+// RUN:     -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
 // CHECK-UNSAFE-MATH: "-cc1"
 // CHECK-UNSAFE-MATH: "-menable-unsafe-fp-math"
 // CHECK-UNSAFE-MATH: "-mreassociate"
 //
 // RUN: %clang -### -fno-fast-math -fno-math-errno -fassociative-math -freciprocal-math \
-// RUN:     -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \
+// RUN:     -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH-UNSAFE-MATH %s
 // CHECK-NO-FAST-MATH-UNSAFE-MATH: "-cc1"
 // CHECK-NO-FAST-MATH-UNSAFE-MATH: "-menable-unsafe-fp-math"
 // CHECK-NO-FAST-MATH-UNSAFE-MATH: "-mreassociate"
 
 // The 2nd -fno-fast-math overrides -fassociative-math.
 
 // RUN: %clang -### -fno-fast-math -fno-math-errno -fassociative-math -freciprocal-math \

 // RUN: %clang -### -ffast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH %s
 // RUN: %clang -### -fno-fast-math -ffast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH %s
 // RUN: %clang -### -funsafe-math-optimizations -ffinite-math-only \
 // RUN:     -fno-math-errno -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH %s
 // RUN: %clang -### -fno-honor-infinities -fno-honor-nans -fno-math-errno \
-// RUN:     -fassociative-math -freciprocal-math -fno-signed-zeros \
+// RUN:     -fassociative-math -freciprocal-math -fno-signed-zeros -fapprox-func \
 // RUN:     -fno-trapping-math -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH %s
 // CHECK-FAST-MATH: "-cc1"
 // CHECK-FAST-MATH: "-ffast-math"
 // CHECK-FAST-MATH: "-ffinite-math-only"
 //
 // RUN: %clang -### -ffast-math -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s

clang/test/Preprocessor/aarch64-target-features.c

 // RUN: %clang -target arm64-none-linux-gnu -mcrc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8.1-a -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // CHECK-CRC32: __ARM_FEATURE_CRC32 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno -fno-signed-zeros\
-// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\
 // RUN:        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // RUN: %clang -target aarch64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // RUN: %clang -target arm64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // CHECK-FASTMATH: __ARM_FP_FAST 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTWCHAR %s
 // RUN: %clang -target arm64-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTWCHAR %s
 // CHECK-SHORTWCHAR: __ARM_SIZEOF_WCHAR_T 2

clang/test/Preprocessor/arm-target-features.c

 // V8M-MAINLINE-DSP-ALLOW-FP-INSTR: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
 
 // RUN: %clang -target arm-none-linux-gnu -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-DEFS %s
 // CHECK-DEFS:#define __ARM_PCS 1
 // CHECK-DEFS:#define __ARM_SIZEOF_MINIMAL_ENUM 4
 // CHECK-DEFS:#define __ARM_SIZEOF_WCHAR_T 4
 
 // RUN: %clang -target arm-none-linux-gnu -fno-math-errno -fno-signed-zeros\
-// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\
 // RUN:        -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s
 // RUN: %clang -target arm-none-linux-gnu -ffast-math -x c -E -dM %s -o -\
 // RUN:        | FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s
 // CHECK-FASTMATH: #define __ARM_FP_FAST 1
 
 // RUN: %clang -target arm-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-SHORTWCHAR %s
 // CHECK-SHORTWCHAR:#define __ARM_SIZEOF_WCHAR_T 2