<div dir="ltr">Looks fine.<div><br></div><div>-Eli</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 6, 2013 at 4:10 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Eli,<br>
<br>
Here's the first patch -- adding of the missing 'n' (nothrow) flag to the existing libm LIBBUILTIN definitions (plus a test case -- unfortunately making the source also valid C++ seems to make the test case uglier).<br>
<br>
Thanks again,<br>
Hal<br>
<br>
----- Original Message -----<br>
><br>
> On Fri, Sep 6, 2013 at 3:53 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> > wrote:<br>
><br>
><br>
><br>
><br>
><br>
><br>
> ----- Original Message -----<br>
> ><br>
> > On Fri, Sep 6, 2013 at 3:32 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> > ><br>
> > > On Fri, Sep 6, 2013 at 8:27 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > > wrote:<br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > Ben, Chad, et al.,<br>
> > ><br>
> > > The attached patch cleans up the LIBBUILTIN libm function<br>
> > > definitions, and improves the way in which -ftrapping-math is<br>
> > > handled in a complementary way. Specifically, this patch:<br>
> > ><br>
> > > 1. The libm LIBBUILTIN definitions are synchronized with the<br>
> > > __builtin_* definitions for libm functions. We currently have<br>
> > > __builtin_* definitions for many more functions than those for<br>
> > > which<br>
> > > we have LIBBUILTIN definitions. This unnecessarily pessimizes<br>
> > > code<br>
> > > generation around many of the remaining libm functions.<br>
> > ><br>
> > > 2. Many of these functions can be marked as readnone so long as<br>
> > > we<br>
> > > can neglect floating point exceptions. I've added a new specifier<br>
> > > 'T', which like 'e' for errno, marks the builtin as const so long<br>
> > > as<br>
> > > trapping math is disabled. I've added a TrappingMath lang option<br>
> > > to<br>
> > > track this.<br>
> > ><br>
> > > 3. Disables -ftrapping-math by default. This is currently enabled<br>
> > > by<br>
> > > default (following gcc), but this is silly. No system of which<br>
> > > I'm<br>
> > > aware actually traps on floating-point exceptions by default<br>
> > > (some<br>
> > > system-specific mechanism is necessary in order to enable SIGFPE<br>
> > > generation). In addition, in Clang, we currently don't support<br>
> > > fenv<br>
> > > access. As a result, we can turn this off by default (and enable<br>
> > > better code generation around the functions for which we have<br>
> > > LIBBUILTIN definitions).<br>
> > ><br>
> > > 4. Updates the CodeGen/libcall-declarations.c test case.<br>
> > ><br>
> > > As a side effect, the existing libm LIBBUILTIN definitions that<br>
> > > were<br>
> > > missing the 'n' (nothrow) specifier now have it.<br>
> > ><br>
> > ><br>
> > ><br>
> > > For clarity, please split the patch into four separate patches:<br>
> > > reformatting, adding missing 'n' specifiers, the<br>
> > > trapping-math-related changes, and adding new LIBBUILTIN<br>
> > > definitions<br>
> > > (preferably in that order). As it is, the Builtins.def changes<br>
> > > are<br>
> > > completely unreviewable.<br>
> ><br>
> > Unfortunately, because almost all of the LIBBUILTIN's in the patch<br>
> > are new, I don't think that splitting the patch will help that :(<br>
> > --<br>
> > FWIW, I think that reviewing the Builtins.def changes means, in<br>
> > practice (either trusting me, or) going through the documentation<br>
> > on<br>
> > each of those functions to make sure that the flags are correct. I<br>
> > don't think that splitting the patch makes that easier either.<br>
> ><br>
> > Nevertheless, I'll split the changes and re-post as separate<br>
> > patches.<br>
> > Maybe I'm wrong ;)<br>
> ><br>
> ><br>
> ><br>
> > I didn't try to count how many were new... it looked like you were<br>
> > changing a bunch of existing ones, but if the balance is as you<br>
> > say,<br>
> > it might not be as useful as I'd hope.<br>
><br>
> There may have been a few changes to the existing ones, I'll go over<br>
> them again, and try to make that more clear in the splitting.<br>
><br>
><br>
> ><br>
> ><br>
> ><br>
> > ><br>
> > > Your changes could be substantially simplified if we don't allow<br>
> > > enabling -ftrapping-math; as you've noted, we don't honor it<br>
> > > anyway.<br>
> > > What do you think?<br>
> ><br>
> > I considered that, but I think that we might as well not make the<br>
> > situation worse for users who do enable trapping math for debugging<br>
> > purposes (as I do myself on occasion).<br>
> ><br>
> ><br>
> > Debugging how, exactly?<br>
><br>
> If I have an application that is producing NaNs, Inf, etc. where that<br>
> is not expected, I can add something like this:<br>
><br>
> #include <fenv.h><br>
> #if defined(__i386__) && defined(__SSE__)<br>
> #include <xmmintrin.h><br>
> #endif<br>
><br>
> ...<br>
><br>
> #if defined(FE_NOMASK_ENV) && !defined(__INTEL_COMPILER)<br>
> fesetenv(FE_NOMASK_ENV);<br>
> fedisableexcept(/* FE_OVERFLOW | */ FE_UNDERFLOW | FE_INEXACT);<br>
> #elif defined(__i386__) && defined(__SSE__)<br>
> _MM_SET_EXCEPTION_MASK(_MM_GET_EXCEPTION_MASK() &<br>
> ~(_MM_MASK_OVERFLOW|_MM_MASK_INVALID|_MM_MASK_DIV_ZERO));<br>
> #endif<br>
><br>
> to the beginning of the code. Then I should get a much better idea<br>
> (based on when the SIGFPE is delivered) of where the problem is.<br>
><br>
><br>
><br>
> Ah, interesting.<br>
><br>
><br>
><br>
><br>
> ><br>
> ><br>
> > With these changes, we'll mark a lot more libm functions that could<br>
> > trap when exceptions are enabled as readnone (including some which<br>
> > never set errno), and if we ignore -ftrapping-math completely,<br>
> > we'll<br>
> > provide no way of indicating that those functions have side effects<br>
> > when we're interested in floating point exceptions.<br>
> ><br>
> ><br>
> > Providing -ftrapping-math as an option is deceptive at best because<br>
> > clang will break code which expects us to honor -ftrapping-math,<br>
> > even if it doesn't even use any library calls.<br>
><br>
> Fair enough. I'll leave that out for now, and we can always revisit<br>
> it later if someone cares.<br>
><br>
> As a separate patch, we could issue a warning (or an error?) if<br>
> -ftrapping-math is provided?<br>
><br>
><br>
><br>
><br>
> Yes, that's a good idea.<br>
><br>
><br>
> -Eli<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory</font></span></blockquote></div><br></div>