Remove connection between 'ffast-math' and 'ffp-contract'.

[zahiraam]

Summary

Currently the options ‘ffast-math’ and ‘ffp-contract’ are connected. When ‘ffast-math’ is set, ffp-contract is altered this way:
-ffast-math/ Ofast -> ffp-contract=fast
-fno-fast-math -> if ffp-contract= fast then ffp-contract=on else ffp-contract unchanged

This differs from gcc which doesn’t connect the two options.

Connecting these two options in clang, resulted in spurious warnings when the user combines these two options -ffast-math -fno-fast-math; see issue https://github.com/llvm/llvm-project/issues/54625.

The issue is that the ‘ffast-math’ option is an on/off flag, but the ‘ffp-contract’ is an on/off/fast flag. So when ‘fno-fast-math’ is used there is no obvious value for ‘ffp-contract’. What should the value of ffp-contract be for -ffp-contract=fast -fno-fast-math and -ffast-math -ffp-contract=fast -fno-fast-math? The current logic sets ffp-contract back to on in these cases. This doesn’t take into account that the value of ffp-contract is modified by an explicit ffp-contract` option.
This patch is proposing a set of rules to apply when ffp-contract', ffast-math and fno-fast-math are combined. These rules would give the user the expected behavior and no diagnostic would be needed.

See RFC https://discourse.llvm.org/t/rfc-making-ffast-math-option-unrelated-to-ffp-contract-option/61912

[aaron.ballman]

I think these changes should go through a community RFC on Discourse given that they alter the behavior of existing flags.

[zahiraam]

@fhahn @aaron.ballman would you mind taking time for a review for this patch? Thanks.

[aaron.ballman]

I made some suggested documentation changes, but would love to hear more from folks heavily into floating-point options.

clang/docs/UsersManual.rst

1409–1410

1432–1435

1441–1443

1450–1452

[zahiraam]

Thanks @aaron.ballman!

[jcranmer-intel]

Some general comments on the documentation here:

One of the lesser-known issues of -ffast-math is the fact that it (or -funsafe-math-optimizations) causes crtfastmath.o to be linked, which adds a static constructor that sets the FTZ/DAZ bits in MXCSR, affecting not only the current compilation unit but all static and shared libraries included in the program. This behavior deserves to be mentioned somewhere in the documentation. Additionally, the effects of fast-math/unsafe-math-optimizations on fdenormal-fp-math{32} should be noted. I realize this is orthogonal to the actual change you're doing here, but while we're improving the documentation of -f[no-]fast-math, we should probably fix this missing documentation at the same time.

clang/lib/Driver/ToolChains/Clang.cpp

2778

Given that this will only be set to "" or "fast", it probably makes sense to make for this be a bool SeenFfastMathOption variable instead.

[jcranmer-intel]

clang/docs/UsersManual.rst

1430

You can add -fdenormal-fp-math=ieee here.

1453–1455

You can replace this text with saying that -fno-fast-math implies -fdenormal-fp-math=ieee. No need to directly mention DenormalFPMath; instead relate it to the other command line flags that are documented.

1455–1458

crtfastmath.o should probably be mentioned in a separate section, like the "A note about ..." sections, with the text in -fno-fast-math only mentioning that it causes code not to be linked with crtfastmath.o.

[zahiraam]

clang/docs/UsersManual.rst

1453–1455

Not sure that's the change you are proposing?

[jcranmer-intel]

I'm happy with these changes. I'll let Aaron have one last crack at the wording of the documentation, in case there's any minor editorial stuff he'd like to see cleaned up.

[aaron.ballman]

LGTM aside from a few nits.

clang/docs/UsersManual.rst

1406

1455

1683

[lenary]

Reverse ping. This has been accepted, what is the status of landing this?

[zahiraam]

In D123630#3899550, @lenary wrote:

Reverse ping. This has been accepted, what is the status of landing this?

As far as I can tell this has landed.

[lenary]

Oh, can you close this revision and link it to the landed commit please?

clang/docs/UsersManual.rst

.. option:: -ffast-math
    * ``-freciprocal-math``
 
    * ``-fno-signed-zeros``
 
    * ``-fno-trapping-math``
 
    * ``-ffp-contract=fast``
 
+.. option:: -fno-fast-math
+
+   Disable fast-math mode.  This options disables unsafe floating-point
+   optimizations by preventing the compiler to make any tranformations that
+   could affect the results.
+
+   This option implies:
+
+   * ``-fhonor-infinities``
+
+   * ``-fhonor-nans``
+
+   * ``-fmath-errno``
+
+   * ``-fno-finite-math-only``
+
+   * ``-fno-associative-math``
+
+   * ``-fno-reciprocal-math``
+
+   * ``-fsigned-zeros``
+
+   * ``-fno-trapping-math``
+
+   * ``-ffp-contract=on``
+
+   There is ambiguity about what value should ``ffp-contract`` has, when
+   options ``-ffp-contract``, ``-ffast-math`` and ``-fno-fast-math`` are
+   combined. To keep the value of ``ffp-contract`` consistent, we define this
+   set of rules:
+
+   * ``-ffast-math`` sets ``ffp-contract`` to ``fast``.
+
+   * ``-fno-fast-math`` sets ``-ffp-contract`` to ``on`` (``fast`` for CUDA and HIP).
+
+   * If ``-ffast-math`` and ``-ffp-contract`` are both seen, but
+     ``-fno-fast-math`` has not been seen after the last instance of
+     ``-ffast-math``, the ``ffp-contract`` will get the value of whichever option
+     was last seen.
+
+   * If ``-fno-fast-math`` is seen and ``-ffp-contract`` has been seen at least
+     once, the ``ffp-contract`` will get the value of the last seen value of
+     ``-ffp-contract``.
+
+   * If ``-fno-fast-math`` is seen and ``-ffp-contract`` has not been seen, the
+     ``ffp-contract`` setting is determined by the default value of
+     ``ffp-contract``.
+
 .. option:: -fdenormal-fp-math=<value>
 
    Select which denormal numbers the code is permitted to require.
 
    Valid values are:
 
    * ``ieee`` - IEEE 754 denormal numbers
    * ``preserve-sign`` - the sign of a flushed-to-zero number is preserved in the sign of 0
    * ``positive-zero`` - denormals are flushed to positive zero
 
    Defaults to ``ieee``.
 
 .. _opt_fstrict-float-cast-overflow:
.. option:: -f[no-]protect-parens:
    has no effect because the optimizer is prohibited from making unsafe
    transformations.
 
 .. _FLT_EVAL_METHOD:
 
 A note about ``__FLT_EVAL_METHOD__``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 The ``__FLT_EVAL_METHOD__`` is not defined as a traditional macro, and so it
 will not appear when dumping preprocessor macros. Instead, the value
 ``__FLT_EVAL_METHOD__`` expands to is determined at the point of expansion
 either from the value set by the ``-ffp-eval-method`` command line option or
 from the target. This is because the ``__FLT_EVAL_METHOD__`` macro
 cannot expand to the correct evaluation method in the presence of a ``#pragma``
 which alters the evaluation method. An error is issued if
 ``__FLT_EVAL_METHOD__`` is expanded inside a scope modified by
 ``#pragma clang fp eval_method``.
 

clang/lib/Driver/ToolChains/Clang.cpp

static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
   const llvm::DenormalMode DefaultDenormalFP32Math =
       TC.getDefaultDenormalModeForType(Args, JA, &llvm::APFloat::IEEEsingle());
 
   llvm::DenormalMode DenormalFPMath = DefaultDenormalFPMath;
   llvm::DenormalMode DenormalFP32Math = DefaultDenormalFP32Math;
   // CUDA and HIP don't rely on the frontend to pass an ffp-contract option.
   // If one wasn't given by the user, don't pass it here.
   StringRef FPContract;
+  StringRef LastSeenFfpContractOption;
+  StringRef LastSeenFfastMathOption;
   if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
       !JA.isOffloading(Action::OFK_HIP))
     FPContract = "on";
   bool StrictFPModel = false;
 
   if (const Arg *A = Args.getLastArg(options::OPT_flimited_precision_EQ)) {
     CmdArgs.push_back("-mlimit-float-precision");
     CmdArgs.push_back(A->getValue());
for (const Arg *A : Args) {
     // Validate and pass through -ffp-contract option.
     case options::OPT_ffp_contract: {
       StringRef Val = A->getValue();
       if (PreciseFPModel) {
         // -ffp-model=precise enables ffp-contract=on.
         // -ffp-model=precise sets PreciseFPModel to on and Val to
         // "precise". FPContract is set.
         ;
-      } else if (Val.equals("fast") || Val.equals("on") || Val.equals("off"))
+      } else if (Val.equals("fast") || Val.equals("on") || Val.equals("off")) {
         FPContract = Val;
+        LastSeenFfpContractOption = Val;
+        }
       else
         D.Diag(diag::err_drv_unsupported_option_argument)
            << A->getOption().getName() << Val;
       break;
     }
 
     // Validate and pass through -ffp-model option.
     case options::OPT_ffp_model_EQ:
case options::OPT_ffast_math:
       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";
+      LastSeenFfastMathOption = "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") {
+          !JA.isOffloading(Action::OFK_HIP)) {
+        if (LastSeenFfpContractOption != "") {
+          FPContract = LastSeenFfpContractOption;
+        } else if (LastSeenFfastMathOption != "")
           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 && !ApproxFunc &&
           DenormalFPMath == llvm::DenormalMode::getIEEE() &&

clang/test/CodeGen/ffp-contract-option.c

 // RUN: %clang_cc1 -no-opaque-pointers -triple=x86_64 -ffp-contract=off -fmath-errno \
 // RUN: -fno-rounding-math %s -emit-llvm -o - \
 // RUN:  -o - | FileCheck --check-prefixes CHECK,CHECK-NOFAST %s
 
 // RUN: %clang -Xclang -no-opaque-pointers -S -emit-llvm -fno-fast-math %s -o - \
 // RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-FPC-ON
 
 // RUN: %clang -Xclang -no-opaque-pointers -S -emit-llvm -ffp-contract=fast -fno-fast-math \
-// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-FPC-ON
+// RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-CONTRACTFAST
 
 // RUN: %clang -Xclang -no-opaque-pointers -S -emit-llvm -ffp-contract=on -fno-fast-math \
 // RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-FPC-ON
 
 // RUN: %clang -Xclang -no-opaque-pointers -S -emit-llvm -ffp-contract=off -fno-fast-math \
 // RUN: %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-FPC-OFF
 
 // RUN: %clang -Xclang -no-opaque-pointers -S -emit-llvm -ffp-model=fast -fno-fast-math \

clang/test/Driver/fp-contract.c

+// Test that -ffp-contract is set to the right value when combined with
+// the options -ffast-math and -fno-fast-math.
+
+// RUN: %clang -### -ffast-math -c %s 2>&1  \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+// CHECK-FPC-FAST:     "-ffp-contract=fast"
+
+// RUN: %clang -### -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+// CHECK-FPC-ON:       "-ffp-contract=on"
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+// CHECK-FPC-OFF:      "-ffp-contract=off"
+
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=fast -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+// RUN: %clang -### -ffp-contract=on -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+// RUN: %clang -### -ffp-contract=off -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=fast -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+// RUN: %clang -### -ffp-contract=on -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+// RUN: %clang -### -ffp-contract=off -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+
+// RUN: %clang -### -ffast-math -ffp-contract=fast -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=fast \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=on -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=fast -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffast-math -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -fno-fast-math -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -fno-fast-math -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -fno-fast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+// RUN: %clang -### -fno-fast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=fast -fno-fast-math -ffp-contract=on \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffp-contract=fast -fno-fast-math -ffp-contract=off \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+// RUN: %clang -### -ffp-contract=off -fno-fast-math -ffp-contract=fast \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=off -fno-fast-math -ffp-contract=on \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffp-contract=on -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=off -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=fast -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -ffp-contract=on -ffast-math -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -ffp-contract=off -ffast-math -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
+
+// RUN: %clang -### -ffp-contract=fast -ffast-math -fno-fast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=fast \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
+
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=on \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
+
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=off \
+// RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
+