[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

[Warren Ristow via Phabricator via cfe-commits]

wristow updated this revision to Diff 238933.
wristow added a comment.

Updated patch to correct a comment and fix a typo.

Regarding the point from @spatel :

> This follows the reasoning from the earlier discussion, but after re-reading the gcc comment in particular, I'm wondering whether this is what we really want to do...
>  If `__FAST_MATH__` is purely here for compatibility with gcc, then should we mimic gcc behavior in setting that macro even if we think it's buggy?

Thinking about it, yes, I think we should mimic GCC.  FTR, I've tried some older GCCs via Compiler Explorer, and found them to be consistent in that removing //some// aspects of `-ffast-math` disables the generation of `__FAST_MATH__` and removing other aspects of it does not.  So my previous recollection ("I could have sworn that in the past that for GCC I had seen this sort of disabling of part of the "fast-math set" result in the disabling of the def of `__FAST_MATH__`.") is wrong.  Bottom line, maybe it's not a bug in GCC, and instead it's the desired GCC behavior (just that I cannot find specific documentation of precisely which settings disable the generation of it).

However, I'd call that a bigger question than this `-ffp-contract` aspect of `__FAST_MATH__`, and a separate bug, that can be fixed separately.

> Ie, when we translate these settings to LLVM's FMF, we can still override the -ffast-math flag by checking the -ffp-contract flag (if I'm seeing it correctly, the existing code will pass that alongside -ffast-math when contract is set to on/off).

How to handle this seems like an implementation question.  The code here is just deciding whether or not we intend to pass "-ffast-math" to cc1 (it isn't directly defining `__FAST_MATH__`).  If we do pass it to cc1, then "clang/lib/Frontend/InitPreprocessor.cpp" will pre-define `__FAST_MATH__`:

  if (LangOpts.FastMath)
    Builder.defineMacro("__FAST_MATH__");

It seems to me that a straightforward way to deal with this question is to make the above test more elaborate (that is, if `LangOpts.FastMath` is set, OR whatever the appropriate subset of fast-math switches are set).  If we do that, we don't need to deal with the "umbrella" aspect if "-fast-math", which gets messy.

Which is to say the approach here can stay the same as it currently is (`-ffp-contract=off/on` suppressing the passing of "-ffast-math" to cc1).  Although the comment about `__FAST_MATH__` here in "Clang.cpp" should be changed, if we take this approach.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72675/new/

https://reviews.llvm.org/D72675

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fast-math.c


Index: clang/test/Driver/fast-math.c
===================================================================
--- clang/test/Driver/fast-math.c
+++ clang/test/Driver/fast-math.c
@@ -180,6 +180,21 @@
 // CHECK-FAST-MATH: "-ffast-math"
 // CHECK-FAST-MATH: "-ffinite-math-only"
 //
+// -ffp-contract=off and -ffp-contract=on must disable the fast-math umbrella,
+// and the unsafe-fp-math umbrella (-ffp-contract=fast leaves them enabled).
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
+//
 // 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 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2760,8 +2760,11 @@
   if (MathErrno)
     CmdArgs.push_back("-fmath-errno");
 
+  // If -ffp-contract=off/on has been specified on the command line, then we
+  // must suppress the emission of -ffast-math and -menable-unsafe-fp-math to
+  // cc1.
   if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
-      !TrappingMath)
+      !TrappingMath && !(FPContract.equals("off") || FPContract.equals("on")))
     CmdArgs.push_back("-menable-unsafe-fp-math");
 
   if (!SignedZeros)
@@ -2804,7 +2807,8 @@
   // that's consistent with gcc's behaviour.
   if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
       ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
-    CmdArgs.push_back("-ffast-math");
+    if (!(FPContract.equals("off") || FPContract.equals("on")))
+      CmdArgs.push_back("-ffast-math");
     if (FPModel.equals("fast")) {
       if (FPContract.equals("fast"))
         // All set, do nothing.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72675.238933.patch
Type: text/x-patch
Size: 2532 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200118/238bea8c/attachment-0001.bin>