[Driver] Clean up denormal handling with fast-math-related options #89477

andykaylor · 2024-04-20T01:02:17Z

This change cleans up the clang driver handling of umbrella options like
-ffast-math, -funsafe-math-optimizations, and -ffp-model, and aligns the
behavior of -ffp-model=fast with -ffast-math with regard to the linking of
crtfastmath.o.

We agreed in a previous review that the fast-math options should not
attempt to change the -fdenormal-fp-math option, which is inherently
target-specific.

The clang user's manual claims that -ffp-model=fast "behaves identically
to specifying both -ffast-math and -ffp-contract=fast." Since -ffast-math
causes crtfastmath.o to be linked if it is available, that should also
happen with -ffp-model=fast.

I am going to be proposing further changes to -ffp-model=fast,
decoupling it from -ffast-math and introducing a new
-ffp-model=aggressive that matches the current behavior, but I wanted
to solidfy the current behavior before I do that.

llvmbot · 2024-04-20T01:02:50Z

@llvm/pr-subscribers-clang-driver

Author: Andy Kaylor (andykaylor)

Changes

This is an attempt to better align the denormal handling of
-ffp-model=fast with the behavior of -ffast-math. The clang user's
manual claims that -ffp-model=fast "behaves identically to specifying
both -ffast-math and -ffp-contract=fast." That isn't entirely correct.
One difference is that -ffast-math causes crtfastmath.o to be linked if
it is available, and passes -fdenormal-fp-math=PreserveSign as a -cc1
option when crtfastmath.o is available.

I'm not sure the current behavior is reasonable and consistent for all
tool chains, but I'm not trying to fix that here. This is just trying to
make an incremental improvement.

I am going to be proposing further changes to -ffp-model=fast,
decoupling it from -ffast-math and introducing a new
-ffp-model=aggressive that matches the current behavior, but I wanted
to solidfy the current behavior before I do that.

Full diff: https://github.com/llvm/llvm-project/pull/89477.diff

6 Files Affected:

(modified) clang/docs/UsersManual.rst (+2-2)
(modified) clang/lib/Driver/ToolChain.cpp (+7-1)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+4)
(modified) clang/test/Driver/default-denormal-fp-math.c (+3)
(modified) clang/test/Driver/linux-ld.c (+3)
(modified) clang/test/Driver/solaris-ld.c (+3)

diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index c464bc3a69adc5..00bb1e779308ef 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -1452,8 +1452,8 @@ floating point semantic models: precise (the default), strict, and fast.
   "fenv_access", "off", "on", "off"
   "rounding_mode", "tonearest", "dynamic", "tonearest"
   "contract", "on", "off", "fast"
-  "denormal_fp_math", "IEEE", "IEEE", "IEEE"
-  "denormal_fp32_math", "IEEE","IEEE", "IEEE"
+  "denormal_fp_math", "IEEE", "IEEE", "target-dependent"
+  "denormal_fp32_math", "IEEE","IEEE", "target-dependent"
   "support_math_errno", "on", "on", "off"
   "no_honor_nans", "off", "off", "on"
   "no_honor_infinities", "off", "off", "on"
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 237092ed07e5dc..a36d8eb1750a2d 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1314,11 +1314,17 @@ bool ToolChain::isFastMathRuntimeAvailable(const ArgList &Args,
     Arg *A =
       Args.getLastArg(options::OPT_ffast_math, options::OPT_fno_fast_math,
                       options::OPT_funsafe_math_optimizations,
-                      options::OPT_fno_unsafe_math_optimizations);
+                      options::OPT_fno_unsafe_math_optimizations,
+                      options::OPT_ffp_model_EQ);
 
     if (!A || A->getOption().getID() == options::OPT_fno_fast_math ||
         A->getOption().getID() == options::OPT_fno_unsafe_math_optimizations)
       return false;
+    if (A && A->getOption().getID() == options::OPT_ffp_model_EQ) {
+      StringRef Model = A->getValue();
+      if (!Model.equals("fast"))
+        return false;
+    }
   }
   // If crtfastmath.o exists add it to the arguments.
   Path = GetFilePath("crtfastmath.o");
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 456ea74caadb00..0ee74855891b03 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2942,6 +2942,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (Val.equals("fast")) {
         FPModel = Val;
         applyFastMath();
+        // The target-specific getDefaultDenormalModeForType handler should
+        // account for -ffp-model=fast and choose its behavior
+        DenormalFPMath = DefaultDenormalFPMath;
+        DenormalFP32Math = DefaultDenormalFP32Math;
       } else if (Val.equals("precise")) {
         optID = options::OPT_ffp_contract;
         FPModel = Val;
diff --git a/clang/test/Driver/default-denormal-fp-math.c b/clang/test/Driver/default-denormal-fp-math.c
index 5f87e151df49e4..61787a8bbe82b8 100644
--- a/clang/test/Driver/default-denormal-fp-math.c
+++ b/clang/test/Driver/default-denormal-fp-math.c
@@ -5,12 +5,15 @@
 
 // crtfastmath enables ftz and daz
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
 
 // crt not linked in with nostartfiles
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math -nostartfiles --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast -nostartfiles --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
 
 // If there's no crtfastmath, don't assume ftz/daz
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math --sysroot=/dev/null -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast --sysroot=/dev/null -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
 
 // RUN: %clang -### -target x86_64-scei-ps4 -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
 
diff --git a/clang/test/Driver/linux-ld.c b/clang/test/Driver/linux-ld.c
index d918f4f2d7dbd9..a3ad564082b966 100644
--- a/clang/test/Driver/linux-ld.c
+++ b/clang/test/Driver/linux-ld.c
@@ -1417,6 +1417,9 @@
 // RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -funsafe-math-optimizations\
 // RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
+// RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -ffp-model=fast \
+// RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
 // RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -Ofast\
 // RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
diff --git a/clang/test/Driver/solaris-ld.c b/clang/test/Driver/solaris-ld.c
index 6d74389e89222c..ce0728d392bf23 100644
--- a/clang/test/Driver/solaris-ld.c
+++ b/clang/test/Driver/solaris-ld.c
@@ -193,6 +193,9 @@
 // RUN: %clang --target=sparc-sun-solaris2.11 -### %s -ffast-math \
 // RUN:        --sysroot=%S/Inputs/solaris_sparc_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH-SPARC32 %s
+// RUN: %clang --target=sparc-sun-solaris2.11 -### %s -ffp-model=fast \
+// RUN:        --sysroot=%S/Inputs/solaris_sparc_tree 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH-SPARC32 %s
 // CHECK-CRTFASTMATH-SPARC32: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-CRTFASTMATH-SPARC32: "[[SYSROOT]]/usr/gcc/4.8/lib/gcc/sparc-sun-solaris2.11/4.8.2{{/|\\\\}}crtfastmath.o"
 // CHECK-NOCRTFASTMATH-SPARC32-NOT: crtfastmath.o

llvmbot · 2024-04-20T01:02:51Z

@llvm/pr-subscribers-clang

Author: Andy Kaylor (andykaylor)

Changes

This is an attempt to better align the denormal handling of
-ffp-model=fast with the behavior of -ffast-math. The clang user's
manual claims that -ffp-model=fast "behaves identically to specifying
both -ffast-math and -ffp-contract=fast." That isn't entirely correct.
One difference is that -ffast-math causes crtfastmath.o to be linked if
it is available, and passes -fdenormal-fp-math=PreserveSign as a -cc1
option when crtfastmath.o is available.

I'm not sure the current behavior is reasonable and consistent for all
tool chains, but I'm not trying to fix that here. This is just trying to
make an incremental improvement.

I am going to be proposing further changes to -ffp-model=fast,
decoupling it from -ffast-math and introducing a new
-ffp-model=aggressive that matches the current behavior, but I wanted
to solidfy the current behavior before I do that.

Full diff: https://github.com/llvm/llvm-project/pull/89477.diff

6 Files Affected:

(modified) clang/docs/UsersManual.rst (+2-2)
(modified) clang/lib/Driver/ToolChain.cpp (+7-1)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+4)
(modified) clang/test/Driver/default-denormal-fp-math.c (+3)
(modified) clang/test/Driver/linux-ld.c (+3)
(modified) clang/test/Driver/solaris-ld.c (+3)

diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index c464bc3a69adc5..00bb1e779308ef 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -1452,8 +1452,8 @@ floating point semantic models: precise (the default), strict, and fast.
   "fenv_access", "off", "on", "off"
   "rounding_mode", "tonearest", "dynamic", "tonearest"
   "contract", "on", "off", "fast"
-  "denormal_fp_math", "IEEE", "IEEE", "IEEE"
-  "denormal_fp32_math", "IEEE","IEEE", "IEEE"
+  "denormal_fp_math", "IEEE", "IEEE", "target-dependent"
+  "denormal_fp32_math", "IEEE","IEEE", "target-dependent"
   "support_math_errno", "on", "on", "off"
   "no_honor_nans", "off", "off", "on"
   "no_honor_infinities", "off", "off", "on"
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 237092ed07e5dc..a36d8eb1750a2d 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1314,11 +1314,17 @@ bool ToolChain::isFastMathRuntimeAvailable(const ArgList &Args,
     Arg *A =
       Args.getLastArg(options::OPT_ffast_math, options::OPT_fno_fast_math,
                       options::OPT_funsafe_math_optimizations,
-                      options::OPT_fno_unsafe_math_optimizations);
+                      options::OPT_fno_unsafe_math_optimizations,
+                      options::OPT_ffp_model_EQ);
 
     if (!A || A->getOption().getID() == options::OPT_fno_fast_math ||
         A->getOption().getID() == options::OPT_fno_unsafe_math_optimizations)
       return false;
+    if (A && A->getOption().getID() == options::OPT_ffp_model_EQ) {
+      StringRef Model = A->getValue();
+      if (!Model.equals("fast"))
+        return false;
+    }
   }
   // If crtfastmath.o exists add it to the arguments.
   Path = GetFilePath("crtfastmath.o");
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 456ea74caadb00..0ee74855891b03 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2942,6 +2942,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (Val.equals("fast")) {
         FPModel = Val;
         applyFastMath();
+        // The target-specific getDefaultDenormalModeForType handler should
+        // account for -ffp-model=fast and choose its behavior
+        DenormalFPMath = DefaultDenormalFPMath;
+        DenormalFP32Math = DefaultDenormalFP32Math;
       } else if (Val.equals("precise")) {
         optID = options::OPT_ffp_contract;
         FPModel = Val;
diff --git a/clang/test/Driver/default-denormal-fp-math.c b/clang/test/Driver/default-denormal-fp-math.c
index 5f87e151df49e4..61787a8bbe82b8 100644
--- a/clang/test/Driver/default-denormal-fp-math.c
+++ b/clang/test/Driver/default-denormal-fp-math.c
@@ -5,12 +5,15 @@
 
 // crtfastmath enables ftz and daz
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
 
 // crt not linked in with nostartfiles
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math -nostartfiles --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast -nostartfiles --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
 
 // If there's no crtfastmath, don't assume ftz/daz
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math --sysroot=/dev/null -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast --sysroot=/dev/null -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-IEEE %s
 
 // RUN: %clang -### -target x86_64-scei-ps4 -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
 
diff --git a/clang/test/Driver/linux-ld.c b/clang/test/Driver/linux-ld.c
index d918f4f2d7dbd9..a3ad564082b966 100644
--- a/clang/test/Driver/linux-ld.c
+++ b/clang/test/Driver/linux-ld.c
@@ -1417,6 +1417,9 @@
 // RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -funsafe-math-optimizations\
 // RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
+// RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -ffp-model=fast \
+// RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
 // RUN: %clang --target=x86_64-unknown-linux -no-pie -### %s -Ofast\
 // RUN:        --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH %s
diff --git a/clang/test/Driver/solaris-ld.c b/clang/test/Driver/solaris-ld.c
index 6d74389e89222c..ce0728d392bf23 100644
--- a/clang/test/Driver/solaris-ld.c
+++ b/clang/test/Driver/solaris-ld.c
@@ -193,6 +193,9 @@
 // RUN: %clang --target=sparc-sun-solaris2.11 -### %s -ffast-math \
 // RUN:        --sysroot=%S/Inputs/solaris_sparc_tree 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH-SPARC32 %s
+// RUN: %clang --target=sparc-sun-solaris2.11 -### %s -ffp-model=fast \
+// RUN:        --sysroot=%S/Inputs/solaris_sparc_tree 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CRTFASTMATH-SPARC32 %s
 // CHECK-CRTFASTMATH-SPARC32: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-CRTFASTMATH-SPARC32: "[[SYSROOT]]/usr/gcc/4.8/lib/gcc/sparc-sun-solaris2.11/4.8.2{{/|\\\\}}crtfastmath.o"
 // CHECK-NOCRTFASTMATH-SPARC32-NOT: crtfastmath.o

github-actions · 2024-04-20T01:05:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

h-vetinari · 2024-04-20T07:49:17Z

This effort is highly desirable, c.f. this blog post, so thanks for that!

Xref: #57589, #80475, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55522

andykaylor · 2024-04-22T17:10:34Z

This effort is highly desirable, c.f. this blog post, so thanks for that!

Xref: #57589, #80475, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55522

My patch here still leaves the problem that we're conflating the "denormal-fp-math" attribute with the desire to set FTZ/DAZ on startup. I think @jcranmer-intel's patch is more important because it actually does something about the shared library problem.

Honestly, my goal here is quite different than what he's doing there. I want to make sure that we are setting FTZ+DAZ during startup when the application entry point is compiled with fast-math enabled. I'd like to eliminate the dependence on crtfastmath.o and set the flags in the FP control register directly in main() when that's what we intended.

I'm not sure what the correct behavior is across all platforms. My perspective on this is heavily X86-biased, so I'd really like some guidance from people who work on other targets about their expectations. I think I saw at least one target that sets denormal-fp-math=preservesign as their default even without any fast-math options enabled. It wouldn't surprise me if there are targets that don't want to set denormal-fp-math=preservesign even with fast-math.

For some(?) Linux targets, we've got the mess I'm touching here where the driver is looking for crtfastmath.o and trying to inspect the command-line for fast-math settings outside the other FP option rendering, and then the FP option rendering, which is meant to be target-independent, is kind of making a mess of it, especially with inconsistent handling of "denormal-fp-math" and "denormal-fp32-math".

zahiraam

LGTM. Thanks.

jcranmer-intel · 2024-04-23T21:09:27Z

I'm not sure what the correct behavior is across all platforms. My perspective on this is heavily X86-biased, so I'd really like some guidance from people who work on other targets about their expectations. I think I saw at least one target that sets denormal-fp-math=preservesign as their default even without any fast-math options enabled. It wouldn't surprise me if there are targets that don't want to set denormal-fp-math=preservesign even with fast-math.

Some of the GPU targets, IIRC, want daz/ftz by default. Not all targets have DAZ/FTZ bits that can be set; I think RISC-V is in this category, although to be honest, trying to track down all the ISA extensions to make sure is a bit beyond my ken.

I kinda do long for the day when daz/ftz can be consigned to the same sort of dustbin of history as, say, EBCDIC.

For some(?) Linux targets, we've got the mess I'm touching here where the driver is looking for crtfastmath.o and trying to inspect the command-line for fast-math settings outside the other FP option rendering, and then the FP option rendering, which is meant to be target-independent, is kind of making a mess of it, especially with inconsistent handling of "denormal-fp-math" and "denormal-fp32-math".

The issue here I think is that of the Linux targets, only the x86 people cared enough to get the denormal-fp-math set correctly, but the logic actually applies across targets. It's a real mess since some targets flush to positive zero and some targets flush to signed zero and some manuals I couldn't make heads or tails out of what they actually did.

arsenm · 2024-04-24T09:02:25Z

Some of the GPU targets, IIRC, want daz/ftz by default. Not all targets have DAZ/FTZ bits that can be set; I think RISC-V is in this category, although to be honest, trying to track down all the ISA extensions to make sure is a bit beyond my ken.

OpenCL allows you to flush float denorms by default and has an explicit flag for it, which is allowed to only change the f32 mode. For AMDGPU we default to no denormals on old targets without fast support. This is also somewhat forced because unfortunately OpenCL only provided a flag to enable flushing and not go in the other direction.

It's a real mess since some targets flush to positive zero and some targets flush to signed zero and some manuals I couldn't make heads or tails out of what they actually did.

I believe ARM has a control bit to set if you want the flush to positive zero or preserve sign mode. I don't see why anyone would ever want the positive zero mode. I'm not sure there are any other users

MaskRay · 2024-04-24T22:01:06Z

clang/lib/Driver/ToolChain.cpp


    if (!A || A->getOption().getID() == options::OPT_fno_fast_math ||
        A->getOption().getID() == options::OPT_fno_unsafe_math_optimizations)
      return false;
+    if (A && A->getOption().getID() == options::OPT_ffp_model_EQ) {
+      StringRef Model = A->getValue();


Nit: A->getValue() != "fast"

inline used-once variable and prefer == != for StringRef, which are much more common than equals.

I hope to add -ffp-model=aggressive soon, with will require a second use of Model here.

andykaylor · 2024-04-24T22:04:41Z

This patch will need extensive changes once #80475 lands. We're basically decoupling "denormal-fp-math" from fast-math, but I think we still want to link "crtfastmath.o" when --fp-model=fast is used, as we do with -ffast-math.

MaskRay · 2024-04-24T22:07:49Z

clang/test/Driver/default-denormal-fp-math.c

@@ -5,12 +5,15 @@

 // crtfastmath enables ftz and daz
 // RUN: %clang -### -target x86_64-unknown-linux-gnu -ffast-math --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s
+// RUN: %clang -### -target x86_64-unknown-linux-gnu -ffp-model=fast --sysroot=%S/Inputs/basic_linux_tree -c %s -v 2>&1 | FileCheck -check-prefix=CHECK-PRESERVESIGN %s


--target= for newer tests. -target has been deprecated since Clang 3.4

MaskRay · 2024-04-24T22:08:19Z

clang/lib/Driver/ToolChains/Clang.cpp

@@ -2942,6 +2942,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
      if (Val.equals("fast")) {
        FPModel = Val;
        applyFastMath();
+        // The target-specific getDefaultDenormalModeForType handler should
+        // account for -ffp-model=fast and choose its behavior


This change cleans up the clang driver handling of umbrella options like -ffast-math, -funsafe-math-optimizations, and -ffp-model, and aligns the behavior of -ffp-model=fast with -ffast-math with regard to the linking of crtfastmath.o. We agreed in a previous review that the fast-math options should not attempt to change the -fdenormal-fp-math option, which is inherently target-specific. The clang user's manual claims that -ffp-model=fast "behaves identically to specifying both -ffast-math and -ffp-contract=fast." Since -ffast-math causes crtfastmath.o to be linked if it is available, that should also happen with -ffp-model=fast. I am going to be proposing further changes to -ffp-model=fast, decoupling it from -ffast-math and introducing a new -ffp-model=aggressive that matches the current behavior, but I wanted to solidfy the current behavior before I do that.

andykaylor · 2024-04-25T19:06:02Z

I've updated this change to account for the changes made in #80475 and made corresponding updated to the PR title and description.

MaskRay · 2024-04-26T00:48:52Z

clang/docs/UsersManual.rst

@@ -1462,6 +1460,14 @@ floating point semantic models: precise (the default), strict, and fast.
  "allow_approximate_fns", "off", "off", "on"
  "allow_reassociation", "off", "off", "on"

+The ``-fp-model`` option does not modify the "fdenormal-fp-math" or
+"fdenormal-fp-math-f32" settings, but it does have an impact on whether


double backticks for the fdenormal-fp-math and crtfastmath.o names

IIRC denormal-fp-math-f32 is only a cc1 flag not exposed to end users. It also is of no use on x86 since it doesn't have a split mode

MaskRay · 2024-04-26T00:50:48Z

clang/test/Driver/fp-model.c

@@ -75,6 +76,7 @@
 // RUN:   --check-prefix=WARN12 %s
 // RUN: %clang -### -ffast-math -ffp-model=strict -c %s 2>&1 | FileCheck \
 // RUN:   --check-prefix=WARN12 %s
+// WARN12: clang


clang is unneeded. For diagnostics or -cc1 options, we just avoid check clang before -cc1 since clang carries very little weight

The only purpose of the check here is to establish a reference point for the WARN12-NOT check on the following line. I can change the thing I'm checking to "-cc1" if that will cause less confusion for people reading the test.

jcranmer-intel

This may need some release notes adjustments as well; I already have a todo to revisit the release notes around release time to make sure we get the summary of the denormal handling flag changes right.

zahiraam · 2024-04-26T18:31:48Z

LGTM. May be no need for the "etc" in the title of the patch?
Thanks.

MaskRay · 2024-04-26T18:42:13Z

clang/test/Driver/fp-model.c

@@ -64,7 +64,8 @@

 // RUN: %clang -### -ffp-model=strict -fdenormal-fp-math=preserve-sign,preserve-sign -c %s 2>&1 \


You can use -### -Werror to ensure that there is no warning.

(In the absence of a --target=, you probably want to check a few different targets that the warning is printed regardless of the default target triple.)

I didn't intend to change the logic of this test. The behavior here should be target-independent now.

MaskRay

Perhaps add a [Driver] tag

andykaylor · 2024-04-26T19:22:20Z

LGTM. May be no need for the "etc" in the title of the patch? Thanks.

The "etc." is eliding -fno-fast-math, -funsafe-math-optimizations, and -fno-unsafe-math-optimizations

jcranmer-intel · 2024-04-26T19:25:07Z

The "etc." is eliding -fno-fast-math, -funsafe-math-optimizations, and -fno-unsafe-math-optimizations

Maybe "fast-math-ish flags" is a good summary of the lot?

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Apr 20, 2024

andykaylor requested review from arsenm, MaskRay, AaronBallman, jcranmer-intel and zahiraam April 20, 2024 01:03

zahiraam approved these changes Apr 23, 2024

View reviewed changes

MaskRay reviewed Apr 24, 2024

View reviewed changes

andykaylor requested a review from jyknight April 24, 2024 22:04

MaskRay reviewed Apr 24, 2024

View reviewed changes

MaskRay approved these changes Apr 24, 2024

View reviewed changes

Andy Kaylor added 3 commits April 25, 2024 11:51

Update FP option rendering to leave denormal-fp-math alone

bf1e013

User manual updates

f1f643c

andykaylor force-pushed the fp-model-fast-ftz branch from cd8df19 to f1f643c Compare April 25, 2024 19:01

andykaylor changed the title ~~Align -ffp-model=fast denormal handling with -ffast-math~~ Clean up denormal handling with -ffp-model, -ffast-math, etc. Apr 25, 2024

Andy Kaylor added 2 commits April 25, 2024 12:09

Formatting fixes

0614850

Fix fp-model.c test and UsersManual.rst

4fc3057

MaskRay reviewed Apr 26, 2024

View reviewed changes

Andy Kaylor added 2 commits April 26, 2024 10:06

Updates based on review feedback

a7e4f5c

Minor documentation edit

fb98b1a

andykaylor requested review from MaskRay and zahiraam April 26, 2024 17:10

jcranmer-intel reviewed Apr 26, 2024

View reviewed changes

MaskRay reviewed Apr 26, 2024

View reviewed changes

MaskRay approved these changes Apr 26, 2024

View reviewed changes

Resolve merge conflict

42e59d2

andykaylor changed the title ~~Clean up denormal handling with -ffp-model, -ffast-math, etc.~~ [Driver] Clean up denormal handling with fast-math-related options Apr 29, 2024

andykaylor merged commit 8ba880b into llvm:main Apr 29, 2024

andykaylor deleted the fp-model-fast-ftz branch August 22, 2024 20:41

		@@ -64,7 +64,8 @@

		// RUN: %clang -### -ffp-model=strict -fdenormal-fp-math=preserve-sign,preserve-sign -c %s 2>&1 \

[Driver] Clean up denormal handling with fast-math-related options #89477

[Driver] Clean up denormal handling with fast-math-related options #89477

Uh oh!

Conversation

andykaylor commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 20, 2024

Uh oh!

llvmbot commented Apr 20, 2024

Uh oh!

github-actions bot commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-vetinari commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andykaylor commented Apr 22, 2024

Uh oh!

zahiraam left a comment

Choose a reason for hiding this comment

Uh oh!

jcranmer-intel commented Apr 23, 2024

Uh oh!

arsenm commented Apr 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor commented Apr 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor commented Apr 25, 2024

Uh oh!

MaskRay Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcranmer-intel left a comment

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Apr 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay left a comment

Choose a reason for hiding this comment

Uh oh!

andykaylor commented Apr 26, 2024

Uh oh!

jcranmer-intel commented Apr 26, 2024

Uh oh!

Uh oh!

andykaylor commented Apr 20, 2024 •

edited

Loading

github-actions bot commented Apr 20, 2024 •

edited

Loading

h-vetinari commented Apr 20, 2024 •

edited

Loading

MaskRay Apr 26, 2024 •

edited

Loading

arsenm Apr 26, 2024 •

edited

Loading