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

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2792
   if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
-      !TrappingMath)
+      !TrappingMath && !(FPContract.equals("off") || FPContract.equals("on")))
     CmdArgs.push_back("-menable-unsafe-fp-math");
----------------
I think this would read better as "... && !FPContract.equals("off") && !FPContract.equals("on")" The '||' in the middle of all these '&&'s combined with the extra parens from the function call trips me up.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2846
       ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
-    CmdArgs.push_back("-ffast-math");
+    if (!(FPContract.equals("off") || FPContract.equals("on")))
+      CmdArgs.push_back("-ffast-math");
----------------
As above, I'd prefer "!off && !on".


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2854
         // Enable -ffp-contract=fast
         CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
       else
----------------
This is a bit of an oddity in our handling. The FPContract local variable starts out initialized to an empty string. So, what we're saying here is that if you've used all the individual controls to set the rest of the fast math flags, we'll turn on fp-contract=fast also. That seems wrong. If you use "-ffast-math", FPContract will have been set to "fast" so that's not applicable here.

I believe this means

```
-fno-honor-infinities -fno-honor-nans -fno-math-errno -fassociative-math -freciprocal-math -fno-signed-zeros -fno-trapping-math -fno-rounding-math

```

will imply "-ffp-contract=fast". Even worse:

```
-ffp-contract=off -fno-fast-math -fno-honor-infinities -fno-honor-nans -fno-math-errno -fassociative-math -freciprocal-math -fno-signed-zeros -fno-trapping-math -fno-rounding-math

```
will imply "-ffp-contract=fast" because "-fno-fast-math" resets FPContract to empty.

I think we should initialize FPContract to whatever we want to be the default (on?) and remove the special case for empty here. Also, -fno-fast-math should either not change FPContract at all or set it to "off". Probably the latter since we're saying -ffast-math includes -ffp-contract=on.


================
Comment at: clang/test/Driver/fast-math.c:196
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
+//
----------------
What about "-ffp-contract=off -ffast-math"? The way the code is written that will override the -ffp-contract option. That's probably what we want, though it might be nice to have a warning.


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

https://reviews.llvm.org/D72675