[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-06 Thread Andy Kaylor via cfe-commits 

https://github.com/andykaylor created 
https://github.com/llvm/llvm-project/pull/91271

This change refactors the fp-contract handling in
RenderFloatingPointOptions in the clang driver code and fixes some
inconsistencies in the way warnings are reported for changes in the
fp-contract behavior.


>From 596ec4fa32dffb9b710ec90d3cafa76fb3bf4b69 Mon Sep 17 00:00:00 2001
From: Andy Kaylor 
Date: Mon, 6 May 2024 10:07:52 -0700
Subject: [PATCH] [Driver] Clean up fp-contract handling in clang driver

This change refactors the fp-contract handling in
RenderFloatingPointOptions in the clang driver code and fixes some
inconsistencies in the way warnings are reported for changes in the
fp-contract behavior.
---
 clang/lib/Driver/ToolChains/Clang.cpp | 90 ---
 clang/test/Driver/fp-contract.c   | 14 +
 clang/test/Driver/fp-model.c  | 14 +
 3 files changed, 81 insertions(+), 37 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 0a2ea96de73824..b1400770f59d53 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2751,6 +2751,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   // If one wasn't given by the user, don't pass it here.
   StringRef FPContract;
   StringRef LastSeenFfpContractOption;
+  StringRef LastFpContractOverrideOption;
   bool SeenUnsafeMathModeOption = false;
   if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
   !JA.isOffloading(Action::OFK_HIP))
@@ -2792,6 +2793,27 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
 SeenUnsafeMathModeOption = true;
   };
 
+  // Lambda to consolidate common handling for fp-contract
+  auto restoreFPContractState = [&]() {
+// CUDA and HIP don't rely on the frontend to pass an ffp-contract option.
+// For other targets, if the state has been changed by one of the
+// unsafe-math umbrella options a subsequent -fno-fast-math or
+// -fno-unsafe-math-optimizations option reverts to the last value seen for
+// the -ffp-contract option or "on" if we have not seen the -ffp-contract
+// option. If we have not seen an unsafe-math option or -ffp-contract,
+// we leave the FPContract state unchanged.
+if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
+!JA.isOffloading(Action::OFK_HIP)) {
+  if (LastSeenFfpContractOption != "")
+FPContract = LastSeenFfpContractOption;
+  else if (SeenUnsafeMathModeOption)
+FPContract = "on";
+}
+// In this case, we're reverting to the last explicit fp-contract option
+// or the platform default
+LastFpContractOverrideOption = "";
+  };
+
   if (const Arg *A = Args.getLastArg(options::OPT_flimited_precision_EQ)) {
 CmdArgs.push_back("-mlimit-float-precision");
 CmdArgs.push_back(A->getValue());
@@ -2893,7 +2915,6 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   AssociativeMath = false;
   ReciprocalMath = false;
   SignedZeros = true;
-  FPContract = "on";
 
   StringRef Val = A->getValue();
   if (OFastEnabled && !Val.equals("fast")) {
@@ -2910,14 +2931,18 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   if (Val.equals("fast")) {
 FPModel = Val;
 applyFastMath();
+// applyFastMath sets fp-contract="fast"
+LastFpContractOverrideOption = "-ffp-model=fast";
   } else if (Val.equals("precise")) {
 FPModel = Val;
 FPContract = "on";
+LastFpContractOverrideOption = "-ffp-model=precise";
   } else if (Val.equals("strict")) {
 StrictFPModel = true;
 FPExceptionBehavior = "strict";
 FPModel = Val;
 FPContract = "off";
+LastFpContractOverrideOption = "-ffp-model=strict";
 TrappingMath = true;
 RoundingFPMath = true;
   } else
@@ -2996,8 +3021,15 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   StringRef Val = A->getValue();
   if (Val.equals("fast") || Val.equals("on") || Val.equals("off") ||
   Val.equals("fast-honor-pragmas")) {
+if (Val != FPContract && LastFpContractOverrideOption != "") {
+  D.Diag(clang::diag::warn_drv_overriding_option)
+  << LastFpContractOverrideOption
+  << Args.MakeArgString("-ffp-contract=" + Val);
+}
+
 FPContract = Val;
 LastSeenFfpContractOption = Val;
+LastFpContractOverrideOption = "";
   } else
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getSpelling() << Val;
@@ -3077,6 +3109,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   TrappingMath = false;
   FPExceptionBehavior = "";
   FPContract = "fast";
+  LastFpContractOverrideOption = "-funsafe-math-optimizations";
   SeenUnsafeMathModeOption

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-06 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-driver

Author: Andy Kaylor (andykaylor)


Changes

This change refactors the fp-contract handling in
RenderFloatingPointOptions in the clang driver code and fixes some
inconsistencies in the way warnings are reported for changes in the
fp-contract behavior.


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


3 Files Affected:

- (modified) clang/lib/Driver/ToolChains/Clang.cpp (+53-37) 
- (modified) clang/test/Driver/fp-contract.c (+14) 
- (modified) clang/test/Driver/fp-model.c (+14) 


``diff
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 0a2ea96de73824..b1400770f59d53 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2751,6 +2751,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   // If one wasn't given by the user, don't pass it here.
   StringRef FPContract;
   StringRef LastSeenFfpContractOption;
+  StringRef LastFpContractOverrideOption;
   bool SeenUnsafeMathModeOption = false;
   if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
   !JA.isOffloading(Action::OFK_HIP))
@@ -2792,6 +2793,27 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
 SeenUnsafeMathModeOption = true;
   };
 
+  // Lambda to consolidate common handling for fp-contract
+  auto restoreFPContractState = [&]() {
+// CUDA and HIP don't rely on the frontend to pass an ffp-contract option.
+// For other targets, if the state has been changed by one of the
+// unsafe-math umbrella options a subsequent -fno-fast-math or
+// -fno-unsafe-math-optimizations option reverts to the last value seen for
+// the -ffp-contract option or "on" if we have not seen the -ffp-contract
+// option. If we have not seen an unsafe-math option or -ffp-contract,
+// we leave the FPContract state unchanged.
+if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
+!JA.isOffloading(Action::OFK_HIP)) {
+  if (LastSeenFfpContractOption != "")
+FPContract = LastSeenFfpContractOption;
+  else if (SeenUnsafeMathModeOption)
+FPContract = "on";
+}
+// In this case, we're reverting to the last explicit fp-contract option
+// or the platform default
+LastFpContractOverrideOption = "";
+  };
+
   if (const Arg *A = Args.getLastArg(options::OPT_flimited_precision_EQ)) {
 CmdArgs.push_back("-mlimit-float-precision");
 CmdArgs.push_back(A->getValue());
@@ -2893,7 +2915,6 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   AssociativeMath = false;
   ReciprocalMath = false;
   SignedZeros = true;
-  FPContract = "on";
 
   StringRef Val = A->getValue();
   if (OFastEnabled && !Val.equals("fast")) {
@@ -2910,14 +2931,18 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   if (Val.equals("fast")) {
 FPModel = Val;
 applyFastMath();
+// applyFastMath sets fp-contract="fast"
+LastFpContractOverrideOption = "-ffp-model=fast";
   } else if (Val.equals("precise")) {
 FPModel = Val;
 FPContract = "on";
+LastFpContractOverrideOption = "-ffp-model=precise";
   } else if (Val.equals("strict")) {
 StrictFPModel = true;
 FPExceptionBehavior = "strict";
 FPModel = Val;
 FPContract = "off";
+LastFpContractOverrideOption = "-ffp-model=strict";
 TrappingMath = true;
 RoundingFPMath = true;
   } else
@@ -2996,8 +3021,15 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   StringRef Val = A->getValue();
   if (Val.equals("fast") || Val.equals("on") || Val.equals("off") ||
   Val.equals("fast-honor-pragmas")) {
+if (Val != FPContract && LastFpContractOverrideOption != "") {
+  D.Diag(clang::diag::warn_drv_overriding_option)
+  << LastFpContractOverrideOption
+  << Args.MakeArgString("-ffp-contract=" + Val);
+}
+
 FPContract = Val;
 LastSeenFfpContractOption = Val;
+LastFpContractOverrideOption = "";
   } else
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getSpelling() << Val;
@@ -3077,6 +3109,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   TrappingMath = false;
   FPExceptionBehavior = "";
   FPContract = "fast";
+  LastFpContractOverrideOption = "-funsafe-math-optimizations";
   SeenUnsafeMathModeOption = true;
   break;
 case options::OPT_fno_unsafe_math_optimizations:
@@ -3084,14 +3117,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   ReciprocalMath = false;
   SignedZeros = true;
   ApproxFunc = false;
-
-  if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
-  !JA.isOffloading(Action::OFK_

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-06 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Andy Kaylor (andykaylor)


Changes

This change refactors the fp-contract handling in
RenderFloatingPointOptions in the clang driver code and fixes some
inconsistencies in the way warnings are reported for changes in the
fp-contract behavior.


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


3 Files Affected:

- (modified) clang/lib/Driver/ToolChains/Clang.cpp (+53-37) 
- (modified) clang/test/Driver/fp-contract.c (+14) 
- (modified) clang/test/Driver/fp-model.c (+14) 


``diff
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 0a2ea96de73824..b1400770f59d53 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2751,6 +2751,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   // If one wasn't given by the user, don't pass it here.
   StringRef FPContract;
   StringRef LastSeenFfpContractOption;
+  StringRef LastFpContractOverrideOption;
   bool SeenUnsafeMathModeOption = false;
   if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
   !JA.isOffloading(Action::OFK_HIP))
@@ -2792,6 +2793,27 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
 SeenUnsafeMathModeOption = true;
   };
 
+  // Lambda to consolidate common handling for fp-contract
+  auto restoreFPContractState = [&]() {
+// CUDA and HIP don't rely on the frontend to pass an ffp-contract option.
+// For other targets, if the state has been changed by one of the
+// unsafe-math umbrella options a subsequent -fno-fast-math or
+// -fno-unsafe-math-optimizations option reverts to the last value seen for
+// the -ffp-contract option or "on" if we have not seen the -ffp-contract
+// option. If we have not seen an unsafe-math option or -ffp-contract,
+// we leave the FPContract state unchanged.
+if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
+!JA.isOffloading(Action::OFK_HIP)) {
+  if (LastSeenFfpContractOption != "")
+FPContract = LastSeenFfpContractOption;
+  else if (SeenUnsafeMathModeOption)
+FPContract = "on";
+}
+// In this case, we're reverting to the last explicit fp-contract option
+// or the platform default
+LastFpContractOverrideOption = "";
+  };
+
   if (const Arg *A = Args.getLastArg(options::OPT_flimited_precision_EQ)) {
 CmdArgs.push_back("-mlimit-float-precision");
 CmdArgs.push_back(A->getValue());
@@ -2893,7 +2915,6 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   AssociativeMath = false;
   ReciprocalMath = false;
   SignedZeros = true;
-  FPContract = "on";
 
   StringRef Val = A->getValue();
   if (OFastEnabled && !Val.equals("fast")) {
@@ -2910,14 +2931,18 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   if (Val.equals("fast")) {
 FPModel = Val;
 applyFastMath();
+// applyFastMath sets fp-contract="fast"
+LastFpContractOverrideOption = "-ffp-model=fast";
   } else if (Val.equals("precise")) {
 FPModel = Val;
 FPContract = "on";
+LastFpContractOverrideOption = "-ffp-model=precise";
   } else if (Val.equals("strict")) {
 StrictFPModel = true;
 FPExceptionBehavior = "strict";
 FPModel = Val;
 FPContract = "off";
+LastFpContractOverrideOption = "-ffp-model=strict";
 TrappingMath = true;
 RoundingFPMath = true;
   } else
@@ -2996,8 +3021,15 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   StringRef Val = A->getValue();
   if (Val.equals("fast") || Val.equals("on") || Val.equals("off") ||
   Val.equals("fast-honor-pragmas")) {
+if (Val != FPContract && LastFpContractOverrideOption != "") {
+  D.Diag(clang::diag::warn_drv_overriding_option)
+  << LastFpContractOverrideOption
+  << Args.MakeArgString("-ffp-contract=" + Val);
+}
+
 FPContract = Val;
 LastSeenFfpContractOption = Val;
+LastFpContractOverrideOption = "";
   } else
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getSpelling() << Val;
@@ -3077,6 +3109,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   TrappingMath = false;
   FPExceptionBehavior = "";
   FPContract = "fast";
+  LastFpContractOverrideOption = "-funsafe-math-optimizations";
   SeenUnsafeMathModeOption = true;
   break;
 case options::OPT_fno_unsafe_math_optimizations:
@@ -3084,14 +3117,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   ReciprocalMath = false;
   SignedZeros = true;
   ApproxFunc = false;
-
-  if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
-  !JA.isOffloading(Action::OFK_HIP)) {

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-07 Thread Zahira Ammarguellat via cfe-commits 


@@ -2893,7 +2915,6 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   AssociativeMath = false;
   ReciprocalMath = false;
   SignedZeros = true;
-  FPContract = "on";

zahiraam wrote:

Not quite sure why this is removed from here?

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-07 Thread Andy Kaylor via cfe-commits 


@@ -2893,7 +2915,6 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   AssociativeMath = false;
   ReciprocalMath = false;
   SignedZeros = true;
-  FPContract = "on";

andykaylor wrote:

It was unnecessary. Only one of the three fp-models sets FPContract to "on" and 
that's happening explicitly below. The code here is setting everything to what 
it would be for fp-model=precise and then resetting it immediately if we're 
handling -ffp-model=fast. Except for the FPContract state, these are right for 
fp-model strict as well.

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-10 Thread Fangrui Song via cfe-commits 


@@ -238,3 +238,17 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations 
\
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF 
%s
 
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \

MaskRay wrote:

L161 has similar `-funsafe-math-optimizations -ffp-contract=off` checks. The 
test can be folded there.

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-10 Thread Fangrui Song via cfe-commits 


@@ -2751,6 +2751,7 @@ static void RenderFloatingPointOptions(const ToolChain 
&TC, const Driver &D,
   // If one wasn't given by the user, don't pass it here.
   StringRef FPContract;
   StringRef LastSeenFfpContractOption;
+  StringRef LastFpContractOverrideOption;

MaskRay wrote:

`Overridden` seems more appropriate, but the variable name will become too long.
I wonder whether `Last` can be removed. It's clear from the type that this can 
only hold one option name.

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-10 Thread Fangrui Song via cfe-commits 


@@ -238,3 +238,17 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations 
\
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF 
%s
 
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \

MaskRay wrote:

I just went ahead and added a bunch of `-Werror` to previous lines in 
514d80b4feea3c788c1b0821959e96f63c8b8f3d

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-10 Thread Andy Kaylor via cfe-commits 


@@ -238,3 +238,17 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations 
\
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF 
%s
 
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \

andykaylor wrote:

That introduces a merge conflict for this patch. Is there a way I can solve 
that without having to do a rebase and force push for this, which kills the 
ability to see what changed between updates? I guess with a small-ish patch 
like this that's not a big problem, but it's something I try to avoid in 
general.

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-05-10 Thread Fangrui Song via cfe-commits 


@@ -238,3 +238,17 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations 
\
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF 
%s
 
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \

MaskRay wrote:

Some folks prefer `git merge origin/main` while some just use `git rebase 
origin/main` and force push.

I am fine with either style, but someone really dislike force push (perhaps not 
for such a trivial change).

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-07-20 Thread Matt Arsenault via cfe-commits 

arsenm wrote:

Can this be closed then? 

https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)

2024-07-22 Thread Andy Kaylor via cfe-commits 

https://github.com/andykaylor closed 
https://github.com/llvm/llvm-project/pull/91271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits