[PATCH] D72675: ix -ffast-math/-ffp-contract interaction

[Warren Ristow via Phabricator via cfe-commits]

wristow marked 3 inline comments as done.
wristow added a comment.

Thanks for the quick feedback @hfinkel



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2721
   if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
-      !TrappingMath)
+      !TrappingMath && !FPContractDisabled)
     CmdArgs.push_back("-menable-unsafe-fp-math");
----------------
hfinkel wrote:
> I think that you just need
> 
>   && !FPContract.equals("off")
> 
> or
> 
>   && !(FPContract.equals("off") || FPContract.equals("on"))
> 
> of which I think the latter. fp-contract=no is also not a fast-math-compatible setting in that regard, right?
Eliminating the `FPContractDisabled` variable, and instead checking that the value isn't "off" (or maybe isn't ("off" or "on")), sounds good.  I'm not 100% sure of whether just "off" or both "off" and "on" ought to be checked.  More on that in the discussion about `__FAST_MATH__` in one of your other comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2745
   if (!FPContract.empty())
     CmdArgs.push_back(Args.MakeArgString("-ffp-contract=" + FPContract));
 
----------------
hfinkel wrote:
> So now we pass it twice, because we also pass it here?
Whoops.  This one was here originally, so I'll leave it here, and delete the one I needlessly added, above.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2763
   if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
       ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
+    if (!FPContractDisabled)
----------------
hfinkel wrote:
> You want the check here, I think, and not below so that if parts of fast-math are disabled `__FAST_MATH__` is not set. That seems to be what the comment/code currently do, although what does GCC do in this regard?
I had originally included the check with the set of conditions of line 2763, but then the `warn_drv_overriding_flag_option` warning (below) was missed in the case of `-ffp-model=fast -ffp-contract=off`.  So I think the check does need to remain inside the above conditional.

Regarding the `__FAST_MATH__` aspect and whether `ffp-contract=on` also ought to suppress the `-ffast-math` emission (and what GCC does), this is somewhat fuzzy for me.  After getting your comments, I'm leaning toward including both the "off" and "on" in the check (rather than only disabling it in the case of "off", as the current patch does).  But I want to at least note my observations here, to make the current situation with both Clang and GCC clear, in case others want to chime in.

Somewhat surprisingly to me, GCC's behavior wrt `__FAST_MATH__` seems buggy in general, so it's not a good base to compare against.  For example, the following set of switches result in the `__FAST_MATH__` macro being defined (although I expected they would have disabled it):
    -ffast-math -fno-associative-math -fno-reciprocal-math -frounding-math

(I've just tested GCC 7.4.0.)

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__`.  As you'd expect with Clang (from the comment in our code code), the above switches //do //suppress the definition of `__FAST_MATH__`.

That said, GCC does suppress the `__FAST_MATH__` def in //some //situations.  E.g., both GCC and Clang suppress it with the following switches:
    -ffast-math -fmath-errno
    -ffast-math -ftrapping-math
    -ffast-math -fsigned-zeros

In any case, for the `-ffp-contract` switch, none of the settings suppress the def of `__FAST_MATH__` with GCC.  That is, all of the following result in `__FAST_MATH__` being defined:
    -ffast-math -ffp-contract=fast
    -ffast-math -ffp-contract=on
    -ffast-math -ffp-contract=off

I think that's buggy (but hard to know if it's `-ffp-contract` specific, or part of the general buggyness of GCC and `__FAST_MATH__`).  Clang (prior to the change described here) also defines `__FAST_MATH__` in all of these `-ffp-contract` situations.  But I think that's just part of the bug I'm trying to address here.

Bottom line: The intended semantics are somewhat fuzzy to me.  I'd say:
    -ffast-math -ffp-contract=fast      // should leave __FAST_MATH__ defined
    -ffast-math -ffp-contract=off       // should not define __FAST_MATH__
    -ffast-math -ffp-contract=on        // unsure ????

For that last one ("unsure ???"), in the current patch I was leaving `__FAST_MATH__` defined.  But I'm now leaning toward also suppressing the `__FAST_MATH__` def in that case.  Unless anyone disagrees, I'll include that change when I update the patch.


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

https://reviews.llvm.org/D72675