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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
