wristow updated this revision to Diff 238933.
wristow added a comment.
Updated patch to correct a comment and fix a typo.
Regarding the point from @spatel :
> This follows the reasoning from the earlier discussion, but after re-reading
> the gcc comment in particular, I'm wondering whether this is what we really
> want to do...
> If `__FAST_MATH__` is purely here for compatibility with gcc, then should we
> mimic gcc behavior in setting that macro even if we think it's buggy?
Thinking about it, yes, I think we should mimic GCC. FTR, I've tried some
older GCCs via Compiler Explorer, and found them to be consistent in that
removing //some// aspects of `-ffast-math` disables the generation of
`__FAST_MATH__` and removing other aspects of it does not. So my previous
recollection ("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__`.") is wrong. Bottom line, maybe it's not a bug in GCC,
and instead it's the desired GCC behavior (just that I cannot find specific
documentation of precisely which settings disable the generation of it).
However, I'd call that a bigger question than this `-ffp-contract` aspect of
`__FAST_MATH__`, and a separate bug, that can be fixed separately.
> Ie, when we translate these settings to LLVM's FMF, we can still override the
> -ffast-math flag by checking the -ffp-contract flag (if I'm seeing it
> correctly, the existing code will pass that alongside -ffast-math when
> contract is set to on/off).
How to handle this seems like an implementation question. The code here is
just deciding whether or not we intend to pass "-ffast-math" to cc1 (it isn't
directly defining `__FAST_MATH__`). If we do pass it to cc1, then
"clang/lib/Frontend/InitPreprocessor.cpp" will pre-define `__FAST_MATH__`:
if (LangOpts.FastMath)
Builder.defineMacro("__FAST_MATH__");
It seems to me that a straightforward way to deal with this question is to make
the above test more elaborate (that is, if `LangOpts.FastMath` is set, OR
whatever the appropriate subset of fast-math switches are set). If we do that,
we don't need to deal with the "umbrella" aspect if "-fast-math", which gets
messy.
Which is to say the approach here can stay the same as it currently is
(`-ffp-contract=off/on` suppressing the passing of "-ffast-math" to cc1).
Although the comment about `__FAST_MATH__` here in "Clang.cpp" should be
changed, if we take this approach.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72675/new/
https://reviews.llvm.org/D72675
Files:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/fast-math.c
Index: clang/test/Driver/fast-math.c
===================================================================
--- clang/test/Driver/fast-math.c
+++ clang/test/Driver/fast-math.c
@@ -180,6 +180,21 @@
// CHECK-FAST-MATH: "-ffast-math"
// CHECK-FAST-MATH: "-ffinite-math-only"
//
+// -ffp-contract=off and -ffp-contract=on must disable the fast-math umbrella,
+// and the unsafe-fp-math umbrella (-ffp-contract=fast leaves them enabled).
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
+//
// RUN: %clang -### -ffast-math -fno-fast-math -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
// RUN: %clang -### -ffast-math -fno-finite-math-only -c %s 2>&1 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2760,8 +2760,11 @@
if (MathErrno)
CmdArgs.push_back("-fmath-errno");
+ // If -ffp-contract=off/on has been specified on the command line, then we
+ // must suppress the emission of -ffast-math and -menable-unsafe-fp-math to
+ // cc1.
if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
- !TrappingMath)
+ !TrappingMath && !(FPContract.equals("off") || FPContract.equals("on")))
CmdArgs.push_back("-menable-unsafe-fp-math");
if (!SignedZeros)
@@ -2804,7 +2807,8 @@
// that's consistent with gcc's behaviour.
if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
- CmdArgs.push_back("-ffast-math");
+ if (!(FPContract.equals("off") || FPContract.equals("on")))
+ CmdArgs.push_back("-ffast-math");
if (FPModel.equals("fast")) {
if (FPContract.equals("fast"))
// All set, do nothing.
Index: clang/test/Driver/fast-math.c
===================================================================
--- clang/test/Driver/fast-math.c
+++ clang/test/Driver/fast-math.c
@@ -180,6 +180,21 @@
// CHECK-FAST-MATH: "-ffast-math"
// CHECK-FAST-MATH: "-ffinite-math-only"
//
+// -ffp-contract=off and -ffp-contract=on must disable the fast-math umbrella,
+// and the unsafe-fp-math umbrella (-ffp-contract=fast leaves them enabled).
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-NO-UNSAFE-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-FAST-MATH %s
+// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \
+// RUN: | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
+//
// RUN: %clang -### -ffast-math -fno-fast-math -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH %s
// RUN: %clang -### -ffast-math -fno-finite-math-only -c %s 2>&1 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2760,8 +2760,11 @@
if (MathErrno)
CmdArgs.push_back("-fmath-errno");
+ // If -ffp-contract=off/on has been specified on the command line, then we
+ // must suppress the emission of -ffast-math and -menable-unsafe-fp-math to
+ // cc1.
if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
- !TrappingMath)
+ !TrappingMath && !(FPContract.equals("off") || FPContract.equals("on")))
CmdArgs.push_back("-menable-unsafe-fp-math");
if (!SignedZeros)
@@ -2804,7 +2807,8 @@
// that's consistent with gcc's behaviour.
if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
- CmdArgs.push_back("-ffast-math");
+ if (!(FPContract.equals("off") || FPContract.equals("on")))
+ CmdArgs.push_back("-ffast-math");
if (FPModel.equals("fast")) {
if (FPContract.equals("fast"))
// All set, do nothing.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
