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

[Warren Ristow via Phabricator via cfe-commits]

wristow marked 4 inline comments as done.
wristow 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");
----------------
andrew.w.kaylor wrote:
> 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.
Sounds good.  Will do.


================
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");
----------------
andrew.w.kaylor wrote:
> As above, I'd prefer "!off && !on".
As above, will do.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2854
         // Enable -ffp-contract=fast
         CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
       else
----------------
andrew.w.kaylor wrote:
> 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.
> This is a bit of an oddity in our handling.

Yes it is!

This is certainly getting more complicated than I had originally expected.  I feel there isn't a clear spec on what we want in terms of whether FMA should be enabled "automatically" at (for example) '-O2', and/or whether it should be enabled by `-ffast-math`.  I'm very willing to make whatever change is needed here, to meet whatever spec we all ultimately agree on.

So I think this patch should be put on hold until we decide on these wider aspects.

One minor note about:
> ... Probably the latter since we're saying -ffast-math includes -ffp-contract=on.

I think that is intended to say "-ffast-math includes -ffp-contract=fast".  The semantics of `-ffp-contract=on` are another part that seems unclear (or at least, more complicated, since it then gets into the FP_CONTRACT pragma).


================
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
+//
----------------
andrew.w.kaylor wrote:
> 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.
Yes, currently `-fffast-math` will override that earlier `-ffp-contract=off` settings.  It's unclear to me whether we're ultimately intending for that to be the behavior (because GCC doesn't do that, as @uweigand noted).  I guess this is another reason to hold off for a bit, until we figure out the wider spec.


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

https://reviews.llvm.org/D72675