[Driver] Restructure handling of -ffast-math and similar options

[john.brawn]

Summary

The way -ffast-math and the various related options to tweak floating-point handling are handled is inflexible and rather confusing. This patch restructures things so that we go through the options adjusting our idea of what's enabled as we go, instead of trying to figure each individual thing out by working backwards from the end, as this makes the behaviour of each individual option more clear.

Doing it this way also means we get gcc-compatible behaviour for when the FAST_MATH and FINITE_MATH_ONLY macros are defined, as they should depend on the final set of features that are enabled and not just on -ffast-math and -ffinite-math-only specifically.

[john.brawn]

Rebase on top of recent driver changes.

[rengolin]

lib/Driver/ToolChains/Clang.cpp

2320 ↗

(On Diff #91311)

format

2324 ↗

(On Diff #91311)

default should be the first item, for readability.

2382 ↗

(On Diff #91311)

Use LLVM_FALLTHROUGH

2452 ↗

(On Diff #91311)

This is technically correct, but users will be confused if they choose -ffast-math -ffp-contract=on and not see -ffast-math coming out on the other side.

Also, fp-contract=on doesn't preclude -ffast-math for the languages that support it, so I wouldn't add FpContract to this list at all.

[john.brawn]

Respond to review comments, and also fix a couple of 80-column violations that I spotted.

[rengolin]

lib/Driver/ToolChains/Clang.cpp

2347 ↗

(On Diff #91745)

Also, when -ffast-math is selected, and -ffp-contract=on, we should actually change it to fast, no?

2356 ↗

(On Diff #91745)

Missing break?

2452 ↗

(On Diff #91311)

I've been thinking a bit more about this, and I started wondering, why do we even need to pass -ffast-math down?

If all the others are already being passed, shouldn't this flag be redundant?

Finally, we could possibly add instead && FpContract != "off". Would that be better?

[john.brawn]

lib/Driver/ToolChains/Clang.cpp

2347 ↗

(On Diff #91745)

Do you mean "clang -ffp-contract=on -ffast-math should set fp-contract to fast" or "clang -ffast-math -ffp-contract=on should set fp-contract to fast"? The first is already done by the -ffast-math handling below, and I think the second is a bad idea because it violates the principle that later command-line options have priority over earlier ones.

2356 ↗

(On Diff #91745)

Whoops, will fix.

2452 ↗

(On Diff #91311)

As the comment above says, -ffast-math enables the FAST_MATH macro.

As to FpContract, going back and checking gcc setting -ffp-contract has no effect on whether FAST_MATH is defined so I think just not checking FpContract here is correct.

[rengolin]

Ok, with the break fix, this looks goof to me. Thanks!

lib/Driver/ToolChains/Clang.cpp

2347 ↗

(On Diff #91745)

The former is done by overall design, the latter is different to most other options because of what "on" means and how it's *not* implemented, meaning it'll be effectively "off". Otherwise, I'd completely agree with you.

But that's not a strong point, for me. I'm happy to not have that now and involve this into a more general "what does on mean" later on.

2452 ↗

(On Diff #91311)

Right, makes sense.

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

#endif
   if (!Args.hasFlag(options::OPT_foptimize_sibling_calls,
                     options::OPT_fno_optimize_sibling_calls))
     CmdArgs.push_back("-mdisable-tail-calls");
 
   // Handle segmented stacks.
   if (Args.hasArg(options::OPT_fsplit_stack))
     CmdArgs.push_back("-split-stacks");
 
-  // If -Ofast is the optimization level, then -ffast-math should be enabled.
-  // This alias option is being used to simplify the getLastArg logic.
-  OptSpecifier FastMathAliasOption =
-      OFastEnabled ? options::OPT_Ofast : options::OPT_ffast_math;
-
   // Handle various floating point optimization flags, mapping them to the
-  // appropriate LLVM code generation flags. The pattern for all of these is to
-  // default off the codegen optimizations, and if any flag enables them and no
-  // flag disables them after the flag enabling them, enable the codegen
-  // optimization. This is complicated by several "umbrella" flags.
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_ffinite_math_only,
-          options::OPT_fno_finite_math_only, options::OPT_fhonor_infinities,
-          options::OPT_fno_honor_infinities))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_finite_math_only &&
-        A->getOption().getID() != options::OPT_fhonor_infinities)
-      CmdArgs.push_back("-menable-no-infs");
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_ffinite_math_only,
-          options::OPT_fno_finite_math_only, options::OPT_fhonor_nans,
-          options::OPT_fno_honor_nans))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_finite_math_only &&
-        A->getOption().getID() != options::OPT_fhonor_nans)
-      CmdArgs.push_back("-menable-no-nans");
-
+  // appropriate LLVM code generation flags. This is complicated by several
+  // "umbrella" flags, so we do this by stepping through the flags incrementally
+  // adjusting what we think is enabled/disabled, then at the end settting the
+  // LLVM flags based on the final state.
+  bool HonorInfs = true;
+  bool HonorNans = true;
   // -fmath-errno is the default on some platforms, e.g. BSD-derived OSes.
   bool MathErrno = getToolChain().IsMathErrnoDefault();
-  if (Arg *A =
-          Args.getLastArg(options::OPT_ffast_math, FastMathAliasOption,
-                          options::OPT_fno_fast_math, options::OPT_fmath_errno,
-                          options::OPT_fno_math_errno)) {
-    // 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).
-    if (A->getOption().getID() == options::OPT_fno_math_errno ||
-        A->getOption().getID() == options::OPT_ffast_math ||
-        A->getOption().getID() == options::OPT_Ofast)
-      MathErrno = false;
-    else if (A->getOption().getID() == options::OPT_fmath_errno)
-      MathErrno = true;
+  bool AssociativeMath = false;
+  bool ReciprocalMath = false;
+  bool SignedZeros = true;
+  bool TrappingMath = true;
+  StringRef DenormalFpMath = "";
+  StringRef FpContract = "";
+
+  for (Arg *A : Args) {
+    switch (A->getOption().getID()) {
+    // 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_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:       TrappingMath = true;     break;
+    case options::OPT_fno_trapping_math:    TrappingMath = false;    break;
+
+    case options::OPT_fdenormal_fp_math_EQ:
+      DenormalFpMath = A->getValue();
+      break;
+
+    // Validate and pass through -fp-contract option.
+    case options::OPT_ffp_contract: {
+      StringRef Val = A->getValue();
+      if (Val == "fast" || Val == "on" || Val == "off") {
+        FpContract = Val;
+      } else {
+        D.Diag(diag::err_drv_unsupported_option_argument)
+            << A->getOption().getName() << Val;
+      }
+      break;
     }
-  if (MathErrno)
-    CmdArgs.push_back("-fmath-errno");
 
-  // There are several flags which require disabling very specific
-  // optimizations. Any of these being disabled forces us to turn off the
-  // entire set of LLVM optimizations, so collect them through all the flag
-  // madness.
-  bool AssociativeMath = false;
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_funsafe_math_optimizations,
-          options::OPT_fno_unsafe_math_optimizations,
-          options::OPT_fassociative_math, options::OPT_fno_associative_math))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_unsafe_math_optimizations &&
-        A->getOption().getID() != options::OPT_fno_associative_math)
+    case options::OPT_ffinite_math_only:
+      HonorInfs = false;
+      HonorNans = false;
+      break;
+    case options::OPT_fno_finite_math_only:
+      HonorInfs = true;
+      HonorNans = true;
+      break;
+
+    case options::OPT_funsafe_math_optimizations:
       AssociativeMath = true;
-  bool ReciprocalMath = false;
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_funsafe_math_optimizations,
-          options::OPT_fno_unsafe_math_optimizations,
-          options::OPT_freciprocal_math, options::OPT_fno_reciprocal_math))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_unsafe_math_optimizations &&
-        A->getOption().getID() != options::OPT_fno_reciprocal_math)
       ReciprocalMath = true;
-  bool SignedZeros = true;
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_funsafe_math_optimizations,
-          options::OPT_fno_unsafe_math_optimizations,
-          options::OPT_fsigned_zeros, options::OPT_fno_signed_zeros))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_unsafe_math_optimizations &&
-        A->getOption().getID() != options::OPT_fsigned_zeros)
       SignedZeros = false;
-  bool TrappingMath = true;
-  if (Arg *A = Args.getLastArg(
-          options::OPT_ffast_math, FastMathAliasOption,
-          options::OPT_fno_fast_math, options::OPT_funsafe_math_optimizations,
-          options::OPT_fno_unsafe_math_optimizations,
-          options::OPT_ftrapping_math, options::OPT_fno_trapping_math))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_unsafe_math_optimizations &&
-        A->getOption().getID() != options::OPT_ftrapping_math)
       TrappingMath = false;
+      break;
+    case options::OPT_fno_unsafe_math_optimizations:
+      AssociativeMath = false;
+      ReciprocalMath = false;
+      SignedZeros = true;
+      TrappingMath = true;
+      // -fno_unsafe_math_optimizations restores default denormal handling
+      DenormalFpMath = "";
+      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;
+      SignedZeros = false;
+      TrappingMath = 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 = getToolChain().IsMathErrnoDefault();
+      AssociativeMath = false;
+      ReciprocalMath = false;
+      SignedZeros = true;
+      TrappingMath = true;
+      // -fno_fast_math restores default denormal and fpcontract handling
+      DenormalFpMath = "";
+      FpContract = "";
+      break;
+    }
+    // If we handled this option claim it
+    A->claim();
+  }
+
+  if (!HonorInfs)
+    CmdArgs.push_back("-menable-no-infs");
+
+  if (!HonorNans)
+    CmdArgs.push_back("-menable-no-nans");
+
+  if (MathErrno)
+    CmdArgs.push_back("-fmath-errno");
+
   if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
       !TrappingMath)
     CmdArgs.push_back("-menable-unsafe-fp-math");
 
   if (!SignedZeros)
     CmdArgs.push_back("-fno-signed-zeros");
 
   if (ReciprocalMath)
     CmdArgs.push_back("-freciprocal-math");
 
   if (!TrappingMath)
     CmdArgs.push_back("-fno-trapping-math");
 
+  if (!DenormalFpMath.empty())
+    CmdArgs.push_back(Args.MakeArgString("-fdenormal-fp-math="+DenormalFpMath));
 
-  if (Arg *A = Args.getLastArg(options::OPT_ffast_math, FastMathAliasOption,
-                               options::OPT_fno_fast_math,
-                               options::OPT_funsafe_math_optimizations,
-                               options::OPT_fno_unsafe_math_optimizations,
-                               options::OPT_fdenormal_fp_math_EQ))
-    if (A->getOption().getID() != options::OPT_fno_fast_math &&
-        A->getOption().getID() != options::OPT_fno_unsafe_math_optimizations)
-      Args.AddLastArg(CmdArgs, options::OPT_fdenormal_fp_math_EQ);
-
-  // Validate and pass through -fp-contract option.
-  if (Arg *A = Args.getLastArg(options::OPT_ffast_math, FastMathAliasOption,
-                               options::OPT_fno_fast_math,
-                               options::OPT_ffp_contract)) {
-    if (A->getOption().getID() == options::OPT_ffp_contract) {
-      StringRef Val = A->getValue();
-      if (Val == "fast" || Val == "on" || Val == "off") {
-        CmdArgs.push_back(Args.MakeArgString("-ffp-contract=" + Val));
-      } else {
-        D.Diag(diag::err_drv_unsupported_option_argument)
-            << A->getOption().getName() << Val;
-      }
-    } else if (A->getOption().matches(options::OPT_ffast_math) ||
-               (OFastEnabled && A->getOption().matches(options::OPT_Ofast))) {
-      // If fast-math is set then set the fp-contract mode to fast.
-      CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
-    }
-  }
+  if (!FpContract.empty())
+    CmdArgs.push_back(Args.MakeArgString("-ffp-contract="+FpContract));
 
   ParseMRecip(getToolChain().getDriver(), Args, CmdArgs);
 
-  // We separately look for the '-ffast-math' and '-ffinite-math-only' flags,
-  // and if we find them, tell the frontend to provide the appropriate
-  // preprocessor macros. This is distinct from enabling any optimizations as
-  // these options induce language changes which must survive serialization
-  // and deserialization, etc.
-  if (Arg *A = Args.getLastArg(options::OPT_ffast_math, FastMathAliasOption,
-                               options::OPT_fno_fast_math))
-    if (!A->getOption().matches(options::OPT_fno_fast_math))
-      CmdArgs.push_back("-ffast-math");
-  if (Arg *A = Args.getLastArg(options::OPT_ffinite_math_only,
-                               options::OPT_fno_fast_math))
-    if (A->getOption().matches(options::OPT_ffinite_math_only))
-      CmdArgs.push_back("-ffinite-math-only");
+  // -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 &&
+      ReciprocalMath && !SignedZeros && !TrappingMath)
+
+  // Handle __FINITE_MATH_ONLY__ similarly.
+  if (!HonorInfs && !HonorNans)
 
   // Decide whether to use verbose asm. Verbose assembly is the default on
   // toolchains which have the integrated assembler on by default.
   bool IsIntegratedAssemblerDefault =
       getToolChain().IsIntegratedAssemblerDefault();
   if (Args.hasFlag(options::OPT_fverbose_asm, options::OPT_fno_verbose_asm,
                    IsIntegratedAssemblerDefault) ||
       Args.hasArg(options::OPT_dA))

cfe/trunk/test/Driver/fast-math.c

 // RUN:   | FileCheck --check-prefix=CHECK-NO-INFS %s
 // RUN: %clang -### -ffinite-math-only -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NANS %s
 // RUN: %clang -### -funsafe-math-optimizations -fno-math-errno -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
 //
 // One umbrella flag is *really* weird and also changes the semantics of the
 // program by adding a special preprocessor macro. Check that the frontend flag
-// modeling this semantic change is provided. Also check that the semantic
-// impact remains even if every optimization is disabled.
+// modeling this semantic change is provided. Also check that the flag is not
+// present if any of the optimization is disabled.
 // 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 -### -ffast-math -fno-finite-math-only \
-// RUN:     -fno-unsafe-math-optimizations -fmath-errno -c %s 2>&1 \
+// RUN: %clang -### -funsafe-math-optimizations -ffinite-math-only \
+// RUN:     -fno-math-errno -ffp-contract=fast -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:     -fno-trapping-math -ffp-contract=fast -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
+// RUN: %clang -### -ffast-math -fno-finite-math-only -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -fno-unsafe-math-optimizations -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -fmath-errno -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
 // CHECK-NO-FAST-MATH: "-cc1"
 // CHECK-NO-FAST-MATH-NOT: "-ffast-math"
 //
 // Check various means of disabling these flags, including disabling them after
 // they've been enabled via an umbrella flag.
 // RUN: %clang -### -fno-honor-infinities -fhonor-infinities -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-INFS %s
 // RUN: %clang -### -ffinite-math-only -fhonor-infinities -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-INFS %s
 // RUN: %clang -### -ffinite-math-only -fno-finite-math-only -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-INFS %s
 // RUN: %clang -### -ffast-math -fhonor-infinities -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-INFS %s
 // RUN: %clang -### -ffast-math -fno-finite-math-only -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-INFS %s
 // CHECK-NO-NO-INFS: "-cc1"
 // CHECK-NO-NO-INFS-NOT: "-menable-no-infs"
+// CHECK-NO-NO-INFS-NOT: "-ffinite-math-only"
 // CHECK-NO-NO-INFS: "-o"
 //
 // RUN: %clang -### -fno-honor-nans -fhonor-nans -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-NANS %s
 // RUN: %clang -### -ffinite-math-only -fhonor-nans -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-NANS %s
 // RUN: %clang -### -ffinite-math-only -fno-finite-math-only -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-NANS %s
 // RUN: %clang -### -ffast-math -fhonor-nans -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-NANS %s
 // RUN: %clang -### -ffast-math -fno-finite-math-only -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-NO-NANS %s
 // CHECK-NO-NO-NANS: "-cc1"
 // CHECK-NO-NO-NANS-NOT: "-menable-no-nans"
+// CHECK-NO-NO-NANS-NOT: "-ffinite-math-only"
 // CHECK-NO-NO-NANS: "-o"
 //
 // RUN: %clang -### -fassociative-math -freciprocal-math -fno-signed-zeros \
 // RUN:     -fno-trapping-math -fno-associative-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
 // RUN: %clang -### -fassociative-math -freciprocal-math -fno-signed-zeros \
 // RUN:     -fno-trapping-math -fno-reciprocal-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s