Thanks Hal, Chandler,<div><br></div><div>Patch committed with some suggestions incorporated in r<font class="Apple-style-span" face="arial, sans-serif"><span class="Apple-style-span" style="border-collapse:collapse">159794.</span></font></div>
<div><font class="Apple-style-span" face="arial, sans-serif"><span class="Apple-style-span" style="border-collapse:collapse"><br></span></font></div><div><font class="Apple-style-span" face="arial, sans-serif"><span class="Apple-style-span" style="border-collapse:collapse">- Lang.<br>
</span></font><br><div class="gmail_quote">On Thu, Jul 5, 2012 at 5:27 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Arg, hit send too soon...<div><br></div><div><br></div><div><div>Index: include/clang/Basic/LangOptions.h</div><div>===================================================================</div><div>--- include/clang/Basic/LangOptions.h<span style="white-space:pre-wrap"> </span>(revision 159591)</div>
<div>+++ include/clang/Basic/LangOptions.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -56,6 +56,12 @@</div><div> SOB_Trapping // -ftrapv</div><div> };</div><div> </div><div>+ enum FPContractModeTy {</div>
<div>+ FPC_Fast, // Aggressively fuse FP ops (E.g. FMA).</div><div>+ FPC_On, // Form fused FP ops according to FP_CONTRACT rules.</div><div>+ FPC_Off // Form fused FP ops only where result will not be affected.</div>
<div>+ };</div><div>+</div></div><div><br></div><div>I would expect the reverse ordering. 0 -> off, and less-than or greater-than having expected semantics.</div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div><div>Index: lib/Frontend/CompilerInvocation.cpp</div><div>===================================================================</div><div>--- lib/Frontend/CompilerInvocation.cpp<span style="white-space:pre-wrap"> </span>(revision 159591)</div>
<div>+++ lib/Frontend/CompilerInvocation.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -759,6 +759,11 @@</div><div> Res.push_back("-ftrapv-handler", Opts.OverflowHandler);</div>
<div> break;</div><div> }</div><div>+ switch (Opts.getFPContractMode()) {</div><div>+ case LangOptions::FPC_Fast: Res.push_back("-ffp-contract=fast"); break;</div><div>+ case LangOptions::FPC_On: Res.push_back("-ffp-contract=on"); break;</div>
<div>+ case LangOptions::FPC_Off: Res.push_back("-ffp-contract=off"); break;</div><div>+ }</div><div> if (Opts.HeinousExtensions)</div><div> Res.push_back("-fheinous-gnu-extensions");</div><div>
// Optimize is implicit.</div><div>@@ -1975,6 +1980,18 @@</div><div> Diags.Report(diag::err_drv_invalid_value)</div><div> << Args.getLastArg(OPT_fvisibility)->getAsString(Args) << Vis;</div><div>
</div><div>+ if (Arg *A = Args.getLastArg(OPT_ffp_contract)) {</div><div>+ StringRef Val = A->getValue(Args);</div><div>+ if (Val == "fast")</div><div>+ Opts.setFPContractMode(LangOptions::FPC_Fast);</div>
<div>+ else if (Val == "on")</div><div>+ Opts.setFPContractMode(LangOptions::FPC_On);</div><div>+ else if (Val == "off")</div><div>+ Opts.setFPContractMode(LangOptions::FPC_Off);</div>
<div>
+ else</div><div>+ Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;</div><div><br></div><div>The frontend isn't supposed to use diagnostics designated for the driver (err_drv_).</div>
</div><div><br></div><div><div>Index: lib/Driver/Tools.cpp</div><div>===================================================================</div><div>--- lib/Driver/Tools.cpp<span style="white-space:pre-wrap"> </span>(revision 159591)</div>
<div>+++ lib/Driver/Tools.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -1781,6 +1781,24 @@</div><div> !TrappingMath)</div><div> CmdArgs.push_back("-menable-unsafe-fp-math");</div>
<div> </div><div>+</div><div>+ // Validate and pass through -fp-contract option. </div><div>+ if (Arg *A = Args.getLastArg(options::OPT_ffast_math,</div><div>+ options::OPT_ffp_contract)) {</div>
<div>+ if (A->getOption().getID() == options::OPT_ffp_contract) {</div><div>+ StringRef Val = A->getValue(Args);</div><div>+ if (Val == "fast" || Val == "on" || Val == "off") {</div>
<div>+ CmdArgs.push_back(Args.MakeArgString("-ffp-contract=" + Val));</div><div>+ } else {</div><div>+ D.Diag(diag::err_drv_unsupported_option_argument)</div><div>+ << A->getOption().getName() << Val;</div>
<div>+ }</div><div>+ } else { // A is OPT_ffast_math</div><div><br></div><div>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.</div>
<div><br></div><div>Is it awkward to integrate this checking there?</div><div><br></div><div>In particular, it seems like there are several sub-flags of -ffast-math that should still enable this...</div></div><div class="HOEnZb">
<div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Jul 5, 2012 at 5:23 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Couple of nit picks on the patch itself, no real issue with the plan:<div><br></div><div><div>Index: test/CodeGen/fp-contract.c</div><div>===================================================================</div><div>--- test/CodeGen/fp-contract.c<span style="white-space:pre-wrap"> </span>(revision 0)</div>
<div>+++ test/CodeGen/fp-contract.c<span style="white-space:pre-wrap"> </span>(revision 0)</div><div>@@ -0,0 +1,8 @@</div><div>+// RUN: %clang_cc1 -O3 -ffp-contract=fast -triple=powerpc-apple-darwin10 -S -o - %s | FileCheck %s</div>
<div>+</div><div>+float fma_test1(float a, float b, float c) {</div><div>+// CHECK: fmadds</div><div>+ float x = a * b;</div><div>+ float y = x + c;</div><div>+ return y;</div><div>+}</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>Index: include/clang/Driver/Options.td</div><div>===================================================================</div><div>--- include/clang/Driver/Options.td<span style="white-space:pre-wrap"> </span>(revision 159591)</div>
<div>+++ include/clang/Driver/Options.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -410,6 +410,12 @@</div><div> def fno_honor_infinites : Flag<"-fno-honor-infinites">, Alias<fno_honor_infinities>;</div>
<div> def ftrapping_math : Flag<"-ftrapping-math">, Group<f_Group>;</div><div> def fno_trapping_math : Flag<"-fno-trapping-math">, Group<f_Group>;</div><div>+def ffp_contract : Joined<"-ffp-contract=">, Group<f_Group>,</div>
<div>+ Flags<[CC1Option]>,</div><div>+ HelpText<"Form fused FP ops (e.g. FMAs): "</div><div>+ "fast (everywhere) | "</div><div>+ "on (according to FP_CONTRACT pragma, default) | "</div>
<div>+ "off (never fuse)">;</div></div><div><br></div><div>The prevailing style in the options TD files is to maximize the columns w/o much regard for indentation:</div><div><br></div>
<div>def ffp_contract: Joined<'-ffp-contract=">, Group<f_Group>,</div><div> Flags<[CC1Option]>, HelpText<"...................."</div><div> ".....">;</div><div class="gmail_extra">
<br><br><div class="gmail_quote"><div><div>On Mon, Jul 2, 2012 at 2:43 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br></div></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
Hi All,<div><br></div><div>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).</div>
<div><br></div><div>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).</div>
<div><br></div><div>Cheers,</div><div>Lang.</div>
<br></div></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>