[clang] [Driver] Clean up fp-contract handling in clang driver (PR #91271)
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)
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)
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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
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)
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