[cfe-commits] [PATCH] Add -ffp-contract option.
Lang Hames
lhames at gmail.com
Thu Jul 5 18:00:48 PDT 2012
Thanks Hal, Chandler,
Patch committed with some suggestions incorporated in r159794.
- Lang.
On Thu, Jul 5, 2012 at 5:27 PM, Chandler Carruth <chandlerc at google.com>wrote:
> Arg, hit send too soon...
>
>
> Index: include/clang/Basic/LangOptions.h
> ===================================================================
> --- include/clang/Basic/LangOptions.h (revision 159591)
> +++ include/clang/Basic/LangOptions.h (working copy)
> @@ -56,6 +56,12 @@
> SOB_Trapping // -ftrapv
> };
>
> + enum FPContractModeTy {
> + FPC_Fast, // Aggressively fuse FP ops (E.g. FMA).
> + FPC_On, // Form fused FP ops according to FP_CONTRACT rules.
> + FPC_Off // Form fused FP ops only where result will not be
> affected.
> + };
> +
>
> I would expect the reverse ordering. 0 -> off, and less-than or
> greater-than having expected semantics.
>
> Also, not sure if you want to switch in this patch, but the coding
> conventions now clearly suggest naming this FPContractModeKind. Most
> LangOpts just haven't been updated.
>
>
> Index: lib/Frontend/CompilerInvocation.cpp
> ===================================================================
> --- lib/Frontend/CompilerInvocation.cpp (revision 159591)
> +++ lib/Frontend/CompilerInvocation.cpp (working copy)
> @@ -759,6 +759,11 @@
> Res.push_back("-ftrapv-handler", Opts.OverflowHandler);
> break;
> }
> + switch (Opts.getFPContractMode()) {
> + case LangOptions::FPC_Fast: Res.push_back("-ffp-contract=fast"); break;
> + case LangOptions::FPC_On: Res.push_back("-ffp-contract=on"); break;
> + case LangOptions::FPC_Off: Res.push_back("-ffp-contract=off"); break;
> + }
> if (Opts.HeinousExtensions)
> Res.push_back("-fheinous-gnu-extensions");
> // Optimize is implicit.
> @@ -1975,6 +1980,18 @@
> Diags.Report(diag::err_drv_invalid_value)
> << Args.getLastArg(OPT_fvisibility)->getAsString(Args) << Vis;
>
> + if (Arg *A = Args.getLastArg(OPT_ffp_contract)) {
> + StringRef Val = A->getValue(Args);
> + if (Val == "fast")
> + Opts.setFPContractMode(LangOptions::FPC_Fast);
> + else if (Val == "on")
> + Opts.setFPContractMode(LangOptions::FPC_On);
> + else if (Val == "off")
> + Opts.setFPContractMode(LangOptions::FPC_Off);
> + else
> + Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args)
> << Val;
>
> The frontend isn't supposed to use diagnostics designated for the driver
> (err_drv_).
>
> Index: lib/Driver/Tools.cpp
> ===================================================================
> --- lib/Driver/Tools.cpp (revision 159591)
> +++ lib/Driver/Tools.cpp (working copy)
> @@ -1781,6 +1781,24 @@
> !TrappingMath)
> CmdArgs.push_back("-menable-unsafe-fp-math");
>
> +
> + // Validate and pass through -fp-contract option.
> + if (Arg *A = Args.getLastArg(options::OPT_ffast_math,
> + options::OPT_ffp_contract)) {
> + if (A->getOption().getID() == options::OPT_ffp_contract) {
> + StringRef Val = A->getValue(Args);
> + if (Val == "fast" || Val == "on" || Val == "off") {
> + CmdArgs.push_back(Args.MakeArgString("-ffp-contract=" + Val));
> + } else {
> + D.Diag(diag::err_drv_unsupported_option_argument)
> + << A->getOption().getName() << Val;
> + }
> + } else { // A is OPT_ffast_math
>
> I'd rather integrate this with the fast-math and friends below. It's a bit
> unfortunate to check the arglist for an option in two separate places -- it
> becomes easy for them to get out of sync.
>
> Is it awkward to integrate this checking there?
>
> In particular, it seems like there are several sub-flags of -ffast-math
> that should still enable this...
>
>
> On Thu, Jul 5, 2012 at 5:23 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> Couple of nit picks on the patch itself, no real issue with the plan:
>>
>> Index: test/CodeGen/fp-contract.c
>> ===================================================================
>> --- test/CodeGen/fp-contract.c (revision 0)
>> +++ test/CodeGen/fp-contract.c (revision 0)
>> @@ -0,0 +1,8 @@
>> +// RUN: %clang_cc1 -O3 -ffp-contract=fast -triple=powerpc-apple-darwin10
>> -S -o - %s | FileCheck %s
>> +
>> +float fma_test1(float a, float b, float c) {
>> +// CHECK: fmadds
>> + float x = a * b;
>> + float y = x + c;
>> + return y;
>> +}
>>
>> I'd prefer to never check actual codegen from within the Clang test
>> suite. I wonder if there is a way to get the backend libraries to print out
>> their flags / state? Then we could just check that, and leave the actual
>> functional test to the LLC-based tests in LLVM. Anyways, not needed for
>> your patch -- this is a long standing deficit.
>>
>> Index: include/clang/Driver/Options.td
>> ===================================================================
>> --- include/clang/Driver/Options.td (revision 159591)
>> +++ include/clang/Driver/Options.td (working copy)
>> @@ -410,6 +410,12 @@
>> def fno_honor_infinites : Flag<"-fno-honor-infinites">,
>> Alias<fno_honor_infinities>;
>> def ftrapping_math : Flag<"-ftrapping-math">, Group<f_Group>;
>> def fno_trapping_math : Flag<"-fno-trapping-math">, Group<f_Group>;
>> +def ffp_contract : Joined<"-ffp-contract=">, Group<f_Group>,
>> + Flags<[CC1Option]>,
>> + HelpText<"Form fused FP ops (e.g. FMAs): "
>> + "fast (everywhere) | "
>> + "on (according to FP_CONTRACT pragma,
>> default) | "
>> + "off (never fuse)">;
>>
>> The prevailing style in the options TD files is to maximize the columns
>> w/o much regard for indentation:
>>
>> def ffp_contract: Joined<'-ffp-contract=">, Group<f_Group>,
>> Flags<[CC1Option]>, HelpText<"...................."
>> ".....">;
>>
>>
>> On Mon, Jul 2, 2012 at 2:43 PM, Lang Hames <lhames at gmail.com> wrote:
>>
>>> Hi All,
>>>
>>> The attached patch adds a -ffp-contract=<style> option which (mostly)
>>> follows the behavior of GCC's option. Styles are 'fast', for aggressive FMA
>>> formation, 'on' for FP_CONTRACT compliance, and 'off' for no FMA formation
>>> (in the future this might be relaxed to FMA formation in cases where the
>>> result is provably unaffected).
>>>
>>> Feedback would be welcome. In particular, I have toggled the fp-contract
>>> style based on the ffast-math option in Clang::ConstructJob(...) in
>>> lib/Driver/Tools.cpp. Is that the right place to do that kind of option
>>> fiddling? (It seems to be where the other fast-math related floating point
>>> options get handled).
>>>
>>> Cheers,
>>> Lang.
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120705/3930ed3e/attachment.html>
More information about the cfe-commits
mailing list