Skip to content

[Driver] Clean up fp-contract handling in clang driver #99723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 24, 2024
Merged

[Driver] Clean up fp-contract handling in clang driver #99723

merged 3 commits into from
Jul 24, 2024

Conversation

andykaylor
Copy link
Contributor

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.

[Driver] Clean up fp-contract handling in clang driver
869385f 
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.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jul 20, 2024
@andykaylor andykaylor requested a review from MaskRay July 20, 2024 00:04
@llvmbot
Copy link
Member

llvmbot commented Jul 20, 2024

@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/99723.diff

3 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+53-37)
  • (modified) clang/test/Driver/fp-contract.c (+35-21)
  • (modified) clang/test/Driver/fp-model.c (+14)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f7b987bf810c1..b6838f42f1331 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2812,6 +2812,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))
@@ -2853,6 +2854,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());
@@ -2954,7 +2976,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 != "fast") {
@@ -2971,14 +2992,18 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (Val == "fast") {
         FPModel = Val;
         applyFastMath();
+        // applyFastMath sets fp-contract="fast"
+        LastFpContractOverrideOption = "-ffp-model=fast";
       } else if (Val == "precise") {
         FPModel = Val;
         FPContract = "on";
+        LastFpContractOverrideOption = "-ffp-model=precise";
       } else if (Val == "strict") {
         StrictFPModel = true;
         FPExceptionBehavior = "strict";
         FPModel = Val;
         FPContract = "off";
+        LastFpContractOverrideOption = "-ffp-model=strict";
         TrappingMath = true;
         RoundingFPMath = true;
       } else
@@ -3057,8 +3082,15 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       StringRef Val = A->getValue();
       if (Val == "fast" || Val == "on" || Val == "off" ||
           Val == "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;
@@ -3137,6 +3169,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:
@@ -3144,14 +3177,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)) {
-        if (LastSeenFfpContractOption != "") {
-          FPContract = LastSeenFfpContractOption;
-        } else if (SeenUnsafeMathModeOption)
-          FPContract = "on";
-      }
+      restoreFPContractState();
       break;
 
     case options::OPT_Ofast:
@@ -3159,10 +3185,13 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (!OFastEnabled)
         continue;
       [[fallthrough]];
-    case options::OPT_ffast_math: {
+    case options::OPT_ffast_math:
       applyFastMath();
+      if (A->getOption().getID() == options::OPT_Ofast)
+        LastFpContractOverrideOption = "-Ofast";
+      else
+        LastFpContractOverrideOption = "-ffast-math";
       break;
-    }
     case options::OPT_fno_fast_math:
       HonorINFs = true;
       HonorNaNs = true;
@@ -3174,16 +3203,11 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       ReciprocalMath = false;
       ApproxFunc = false;
       SignedZeros = true;
-      // -fno_fast_math restores default fpcontract handling
-      if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
-          !JA.isOffloading(Action::OFK_HIP)) {
-        if (LastSeenFfpContractOption != "") {
-          FPContract = LastSeenFfpContractOption;
-        } else if (SeenUnsafeMathModeOption)
-          FPContract = "on";
-      }
+      restoreFPContractState();
+      LastFpContractOverrideOption = "";
       break;
-    }
+    } // End switch (A->getOption().getID())
+
     // The StrictFPModel local variable is needed to report warnings
     // in the way we intend. If -ffp-model=strict has been used, we
     // want to report a warning for the next option encountered that
@@ -3201,12 +3225,17 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       else {
         StrictFPModel = false;
         FPModel = "";
+        // The warning for -ffp-contract would have been reported by the
+        // OPT_ffp_contract_EQ handler above. A special check here is needed
+        // to avoid duplicating the warning.
         auto RHS = (A->getNumValues() == 0)
                        ? A->getSpelling()
                        : Args.MakeArgString(A->getSpelling() + A->getValue());
-        if (RHS != "-ffp-model=strict")
-          D.Diag(clang::diag::warn_drv_overriding_option)
-              << "-ffp-model=strict" << RHS;
+        if (A->getSpelling() != "-ffp-contract=") {
+          if (RHS != "-ffp-model=strict")
+            D.Diag(clang::diag::warn_drv_overriding_option)
+                << "-ffp-model=strict" << RHS;
+        }
       }
     }
 
@@ -3288,21 +3317,8 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
   // individual features enabled by -ffast-math instead of the option itself as
   // that's consistent with gcc's behaviour.
   if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath && ApproxFunc &&
-      ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
+      ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath)
     CmdArgs.push_back("-ffast-math");
-    if (FPModel == "fast") {
-      if (FPContract == "fast")
-        // All set, do nothing.
-        ;
-      else if (FPContract.empty())
-        // Enable -ffp-contract=fast
-        CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
-      else
-        D.Diag(clang::diag::warn_drv_overriding_option)
-            << "-ffp-model=fast"
-            << Args.MakeArgString("-ffp-contract=" + FPContract);
-    }
-  }
 
   // Handle __FINITE_MATH_ONLY__ similarly.
   if (!HonorINFs && !HonorNaNs)
diff --git a/clang/test/Driver/fp-contract.c b/clang/test/Driver/fp-contract.c
index e2691dc211cc3..978bf80345165 100644
--- a/clang/test/Driver/fp-contract.c
+++ b/clang/test/Driver/fp-contract.c
@@ -10,18 +10,18 @@
 // RUN: %clang -### -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 // CHECK-FPC-ON:       "-ffp-contract=on"
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 // CHECK-FPC-OFF:      "-ffp-contract=off"
 
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=fast-honor-pragmas -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=fast-honor-pragmas -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST-HONOR %s
 // CHECK-FPC-FAST-HONOR:     "-ffp-contract=fast-honor-pragmas"
 
@@ -43,22 +43,22 @@
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -ffp-contract=fast -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -ffp-contract=fast \
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -fno-fast-math -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -fno-fast-math -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -fno-fast-math -c %s 2>&1 \
@@ -112,10 +112,10 @@
 // RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=on \
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=on \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=off \
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=off \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // funsafe-math-optimizations, fno-unsafe-math-optimizations
@@ -125,10 +125,10 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=fast -c %s 2>&1 \
@@ -151,25 +151,25 @@
 // RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=fast \
 // RUN: -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
@@ -229,9 +229,23 @@
 // RUN:   -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -fno-unsafe-math-optimizations -funsafe-math-optimizations \
+// RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations \
 // RUN:   -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -fno-unsafe-math-optimizations -funsafe-math-optimizations \
+// 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 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-funsafe-math-optimizations' option with '-ffp-contract=off'
+
+// This case should not warn
+// RUN: %clang -### -Werror -funsafe-math-optimizations \
+// RUN: -fno-unsafe-math-optimizations -ffp-contract=off -c %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffast-math' option with '-ffp-contract=off'
+
+// This case should not warn
+// RUN: %clang -### -Werror -ffast-math -fno-fast-math -ffp-contract=off -c %s
diff --git a/clang/test/Driver/fp-model.c b/clang/test/Driver/fp-model.c
index 644523394d6b1..8451885669512 100644
--- a/clang/test/Driver/fp-model.c
+++ b/clang/test/Driver/fp-model.c
@@ -81,6 +81,20 @@
 // RUN:   | FileCheck --check-prefix=WARN13 %s
 // WARN13: warning: overriding '-ffp-model=strict' option with '-fapprox-func' [-Woverriding-option]
 
+// RUN: %clang -### -ffp-model=precise -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN14 %s
+// WARN14: warning: overriding '-ffp-model=precise' option with '-ffp-contract=off' [-Woverriding-option]
+
+// RUN: %clang -### -ffp-model=precise -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN15 %s
+// WARN15: warning: overriding '-ffp-model=precise' option with '-ffp-contract=fast' [-Woverriding-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -ffp-contract=on \
+// RUN:   -c %s 2>&1 | FileCheck --check-prefix=WARN16 %s
+// WARN16: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-option]
+// WARN16: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-option]
+
+
 // RUN: %clang -### -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NOROUND %s
 // CHECK-NOROUND: "-cc1"

@llvmbot
Copy link
Member

llvmbot commented Jul 20, 2024

@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/99723.diff

3 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+53-37)
  • (modified) clang/test/Driver/fp-contract.c (+35-21)
  • (modified) clang/test/Driver/fp-model.c (+14)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f7b987bf810c1..b6838f42f1331 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2812,6 +2812,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))
@@ -2853,6 +2854,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());
@@ -2954,7 +2976,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 != "fast") {
@@ -2971,14 +2992,18 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (Val == "fast") {
         FPModel = Val;
         applyFastMath();
+        // applyFastMath sets fp-contract="fast"
+        LastFpContractOverrideOption = "-ffp-model=fast";
       } else if (Val == "precise") {
         FPModel = Val;
         FPContract = "on";
+        LastFpContractOverrideOption = "-ffp-model=precise";
       } else if (Val == "strict") {
         StrictFPModel = true;
         FPExceptionBehavior = "strict";
         FPModel = Val;
         FPContract = "off";
+        LastFpContractOverrideOption = "-ffp-model=strict";
         TrappingMath = true;
         RoundingFPMath = true;
       } else
@@ -3057,8 +3082,15 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       StringRef Val = A->getValue();
       if (Val == "fast" || Val == "on" || Val == "off" ||
           Val == "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;
@@ -3137,6 +3169,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:
@@ -3144,14 +3177,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)) {
-        if (LastSeenFfpContractOption != "") {
-          FPContract = LastSeenFfpContractOption;
-        } else if (SeenUnsafeMathModeOption)
-          FPContract = "on";
-      }
+      restoreFPContractState();
       break;
 
     case options::OPT_Ofast:
@@ -3159,10 +3185,13 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       if (!OFastEnabled)
         continue;
       [[fallthrough]];
-    case options::OPT_ffast_math: {
+    case options::OPT_ffast_math:
       applyFastMath();
+      if (A->getOption().getID() == options::OPT_Ofast)
+        LastFpContractOverrideOption = "-Ofast";
+      else
+        LastFpContractOverrideOption = "-ffast-math";
       break;
-    }
     case options::OPT_fno_fast_math:
       HonorINFs = true;
       HonorNaNs = true;
@@ -3174,16 +3203,11 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       ReciprocalMath = false;
       ApproxFunc = false;
       SignedZeros = true;
-      // -fno_fast_math restores default fpcontract handling
-      if (!JA.isDeviceOffloading(Action::OFK_Cuda) &&
-          !JA.isOffloading(Action::OFK_HIP)) {
-        if (LastSeenFfpContractOption != "") {
-          FPContract = LastSeenFfpContractOption;
-        } else if (SeenUnsafeMathModeOption)
-          FPContract = "on";
-      }
+      restoreFPContractState();
+      LastFpContractOverrideOption = "";
       break;
-    }
+    } // End switch (A->getOption().getID())
+
     // The StrictFPModel local variable is needed to report warnings
     // in the way we intend. If -ffp-model=strict has been used, we
     // want to report a warning for the next option encountered that
@@ -3201,12 +3225,17 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
       else {
         StrictFPModel = false;
         FPModel = "";
+        // The warning for -ffp-contract would have been reported by the
+        // OPT_ffp_contract_EQ handler above. A special check here is needed
+        // to avoid duplicating the warning.
         auto RHS = (A->getNumValues() == 0)
                        ? A->getSpelling()
                        : Args.MakeArgString(A->getSpelling() + A->getValue());
-        if (RHS != "-ffp-model=strict")
-          D.Diag(clang::diag::warn_drv_overriding_option)
-              << "-ffp-model=strict" << RHS;
+        if (A->getSpelling() != "-ffp-contract=") {
+          if (RHS != "-ffp-model=strict")
+            D.Diag(clang::diag::warn_drv_overriding_option)
+                << "-ffp-model=strict" << RHS;
+        }
       }
     }
 
@@ -3288,21 +3317,8 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
   // individual features enabled by -ffast-math instead of the option itself as
   // that's consistent with gcc's behaviour.
   if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath && ApproxFunc &&
-      ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
+      ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath)
     CmdArgs.push_back("-ffast-math");
-    if (FPModel == "fast") {
-      if (FPContract == "fast")
-        // All set, do nothing.
-        ;
-      else if (FPContract.empty())
-        // Enable -ffp-contract=fast
-        CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
-      else
-        D.Diag(clang::diag::warn_drv_overriding_option)
-            << "-ffp-model=fast"
-            << Args.MakeArgString("-ffp-contract=" + FPContract);
-    }
-  }
 
   // Handle __FINITE_MATH_ONLY__ similarly.
   if (!HonorINFs && !HonorNaNs)
diff --git a/clang/test/Driver/fp-contract.c b/clang/test/Driver/fp-contract.c
index e2691dc211cc3..978bf80345165 100644
--- a/clang/test/Driver/fp-contract.c
+++ b/clang/test/Driver/fp-contract.c
@@ -10,18 +10,18 @@
 // RUN: %clang -### -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 // CHECK-FPC-ON:       "-ffp-contract=on"
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 // CHECK-FPC-OFF:      "-ffp-contract=off"
 
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=fast-honor-pragmas -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=fast-honor-pragmas -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST-HONOR %s
 // CHECK-FPC-FAST-HONOR:     "-ffp-contract=fast-honor-pragmas"
 
@@ -43,22 +43,22 @@
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -ffp-contract=fast -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -ffp-contract=fast \
+// RUN: %clang -### -ffast-math -ffp-contract=off -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -fno-fast-math -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=on -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -ffast-math -ffp-contract=off -fno-fast-math -c %s 2>&1 \
+// RUN: %clang -### -ffast-math -ffp-contract=off -fno-fast-math -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // RUN: %clang -### -Werror -ffast-math -ffp-contract=fast -fno-fast-math -c %s 2>&1 \
@@ -112,10 +112,10 @@
 // RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=on \
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=on \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -fno-fast-math -ffast-math -ffp-contract=off \
+// RUN: %clang -### -fno-fast-math -ffast-math -ffp-contract=off \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // funsafe-math-optimizations, fno-unsafe-math-optimizations
@@ -125,10 +125,10 @@
 // RUN: %clang -### -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on -c %s 2>&1 \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
 // RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=fast -c %s 2>&1 \
@@ -151,25 +151,25 @@
 // RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=fast \
 // RUN: -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -ffp-contract=off -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -ffp-contract=fast -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=on \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=on \
 // RUN: -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -funsafe-math-optimizations -ffp-contract=off \
+// RUN: %clang -### -funsafe-math-optimizations -ffp-contract=off \
 // RUN: -fno-unsafe-math-optimizations -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-FPC-OFF %s
 
@@ -229,9 +229,23 @@
 // RUN:   -ffp-contract=fast \
 // RUN: -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-FAST %s
 
-// RUN: %clang -### -Werror -fno-unsafe-math-optimizations -funsafe-math-optimizations \
+// RUN: %clang -### -fno-unsafe-math-optimizations -funsafe-math-optimizations \
 // RUN:   -ffp-contract=on -c %s 2>&1 | FileCheck --check-prefix=CHECK-FPC-ON %s
 
-// RUN: %clang -### -Werror -fno-unsafe-math-optimizations -funsafe-math-optimizations \
+// 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 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-funsafe-math-optimizations' option with '-ffp-contract=off'
+
+// This case should not warn
+// RUN: %clang -### -Werror -funsafe-math-optimizations \
+// RUN: -fno-unsafe-math-optimizations -ffp-contract=off -c %s
+
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffast-math' option with '-ffp-contract=off'
+
+// This case should not warn
+// RUN: %clang -### -Werror -ffast-math -fno-fast-math -ffp-contract=off -c %s
diff --git a/clang/test/Driver/fp-model.c b/clang/test/Driver/fp-model.c
index 644523394d6b1..8451885669512 100644
--- a/clang/test/Driver/fp-model.c
+++ b/clang/test/Driver/fp-model.c
@@ -81,6 +81,20 @@
 // RUN:   | FileCheck --check-prefix=WARN13 %s
 // WARN13: warning: overriding '-ffp-model=strict' option with '-fapprox-func' [-Woverriding-option]
 
+// RUN: %clang -### -ffp-model=precise -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN14 %s
+// WARN14: warning: overriding '-ffp-model=precise' option with '-ffp-contract=off' [-Woverriding-option]
+
+// RUN: %clang -### -ffp-model=precise -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN15 %s
+// WARN15: warning: overriding '-ffp-model=precise' option with '-ffp-contract=fast' [-Woverriding-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -ffp-contract=on \
+// RUN:   -c %s 2>&1 | FileCheck --check-prefix=WARN16 %s
+// WARN16: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-option]
+// WARN16: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-option]
+
+
 // RUN: %clang -### -c %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NOROUND %s
 // CHECK-NOROUND: "-cc1"

@andykaylor
Copy link
Contributor Author

This is a refreshed version of #91271. Given how long it's been since I updated that PR, it seemed better to just start fresh.

MaskRay
MaskRay reviewed Jul 20, 2024
View reviewed changes
clang/test/Driver/fp-contract.c
@@ -10,18 +10,18 @@
// RUN: %clang -### -fno-fast-math -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-FPC-ON %s

// RUN: %clang -### -Werror -ffast-math -ffp-contract=on -c %s 2>&1 \
// RUN: %clang -### -ffast-math -ffp-contract=on -c %s 2>&1 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these RUN lines now emit warnings (upgraded to errors if -Werror), consider adding check prefixes for the warnings.

@zahiraam
Copy link
Contributor

This is a refreshed version of #91271. Given how long it's been since I updated that PR, it seemed better to just start fresh.

May be #91271 should be closed then,
In Driver/fp-contract.c, all the //RUN lines where you removed the -Werror should now have //WARN check lines.

MaskRay
MaskRay reviewed Jul 23, 2024
View reviewed changes
clang/test/Driver/fp-model.c Outdated
// WARN16: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-option]
// WARN16: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-option]


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded blank line

MaskRay
MaskRay approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is complex and difficult to follow, but the test updates looks good!

zahiraam
zahiraam approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@zahiraam zahiraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@andykaylor andykaylor merged commit 97839a8 into llvm:main Jul 24, 2024
7 checks passed
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@yuxuanchen1997
[Driver] Clean up fp-contract handling in clang driver (#99723)
b69fd87 
Summary:
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.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250650
@andykaylor andykaylor deleted the cleanup-fp-contract branch August 22, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaskRay MaskRay MaskRay approved these changes

@zahiraam zahiraam zahiraam approved these changes

@AaronBallman AaronBallman Awaiting requested review from AaronBallman

@arsenm arsenm Awaiting requested review from arsenm

@jcranmer-intel jcranmer-intel Awaiting requested review from jcranmer-intel

Assignees
No one assigned
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andykaylor @llvmbot @zahiraam @MaskRay