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