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