<div dir="ltr">On Fri, Sep 6, 2013 at 5:20 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Eli,<br>
<br>
Here's the second part for review:<br>
<br>
Currently the LIBBUILTIN definitions for atan and atan2 say that these functions might set errno, but they never do (this is the only other relevant functional change to the existing definitions in the original patch).<br>
</blockquote><div><br></div><div><br></div><div><a href="http://pubs.opengroup.org/onlinepubs/9699919799/functions/atan.html">http://pubs.opengroup.org/onlinepubs/9699919799/functions/atan.html</a> says atan can in fact set errno.<br>
</div><div><br></div><div>-Eli</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks again,<br>
Hal<br>
<br>
----- Original Message -----<br>
> ----- Original Message -----<br>
> > ----- Original Message -----<br>
> > ><br>
> > > Looks fine.<br>
> ><br>
> > r190217, thanks!<br>
> ><br>
> > [I also committed, in r190218, a trivial reordering of the existing<br>
> > definitions.]<br>
><br>
> And in r190222, I reordered the existing definitions to match the<br>
> order of the __builtin_* definitions (which is a small change, but<br>
> should make comparing to the new list easier).<br>
><br>
>  -Hal<br>
><br>
> ><br>
> >  -Hal<br>
> ><br>
> > ><br>
> > ><br>
> > > -Eli<br>
> > ><br>
> > ><br>
> > ><br>
> > > On Fri, Sep 6, 2013 at 4:10 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > > wrote:<br>
> > ><br>
> > ><br>
> > > Eli,<br>
> > ><br>
> > > Here's the first patch -- adding of the missing 'n' (nothrow)<br>
> > > flag<br>
> > > to<br>
> > > the existing libm LIBBUILTIN definitions (plus a test case --<br>
> > > unfortunately making the source also valid C++ seems to make the<br>
> > > 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> ><br>
> > > > 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>
> > > > > ><br>
> > > > > wrote:<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > ----- Original Message -----<br>
> > > > > ><br>
> > > > > > On Fri, Sep 6, 2013 at 8:27 AM, Hal Finkel <<br>
> > > > > > <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a><br>
> > > > > > ><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<br>
> > > > > > is<br>
> > > > > > handled in a complementary way. Specifically, this patch:<br>
> > > > > ><br>
> > > > > > 1. The libm LIBBUILTIN definitions are synchronized with<br>
> > > > > > the<br>
> > > > > > __builtin_* definitions for libm functions. We currently<br>
> > > > > > have<br>
> > > > > > __builtin_* definitions for many more functions than those<br>
> > > > > > for<br>
> > > > > > which<br>
> > > > > > we have LIBBUILTIN definitions. This unnecessarily<br>
> > > > > > 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<br>
> > > > > > long<br>
> > > > > > as<br>
> > > > > > we<br>
> > > > > > can neglect floating point exceptions. I've added a new<br>
> > > > > > specifier<br>
> > > > > > 'T', which like 'e' for errno, marks the builtin as const<br>
> > > > > > so<br>
> > > > > > long<br>
> > > > > > as<br>
> > > > > > trapping math is disabled. I've added a TrappingMath lang<br>
> > > > > > option<br>
> > > > > > to<br>
> > > > > > track this.<br>
> > > > > ><br>
> > > > > > 3. Disables -ftrapping-math by default. This is currently<br>
> > > > > > enabled<br>
> > > > > > by<br>
> > > > > > default (following gcc), but this is silly. No system of<br>
> > > > > > which<br>
> > > > > > I'm<br>
> > > > > > aware actually traps on floating-point exceptions by<br>
> > > > > > default<br>
> > > > > > (some<br>
> > > > > > system-specific mechanism is necessary in order to enable<br>
> > > > > > SIGFPE<br>
> > > > > > generation). In addition, in Clang, we currently don't<br>
> > > > > > support<br>
> > > > > > fenv<br>
> > > > > > access. As a result, we can turn this off by default (and<br>
> > > > > > enable<br>
> > > > > > better code generation around the functions for which we<br>
> > > > > > 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<br>
> > > > > > 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<br>
> > > > > > 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<br>
> > > > > > changes<br>
> > > > > > are<br>
> > > > > > completely unreviewable.<br>
> > > > ><br>
> > > > > Unfortunately, because almost all of the LIBBUILTIN's in the<br>
> > > > > patch<br>
> > > > > are new, I don't think that splitting the patch will help<br>
> > > > > that<br>
> > > > > :(<br>
> > > > > --<br>
> > > > > FWIW, I think that reviewing the Builtins.def changes means,<br>
> > > > > in<br>
> > > > > practice (either trusting me, or) going through the<br>
> > > > > documentation<br>
> > > > > on<br>
> > > > > each of those functions to make sure that the flags are<br>
> > > > > correct.<br>
> > > > > I<br>
> > > > > don't think that splitting the patch makes that easier<br>
> > > > > 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<br>
> > > > > were<br>
> > > > > changing a bunch of existing ones, but if the balance is as<br>
> > > > > 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<br>
> > > > 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<br>
> > > > > > allow<br>
> > > > > > enabling -ftrapping-math; as you've noted, we don't honor<br>
> > > > > > it<br>
> > > > > > anyway.<br>
> > > > > > What do you think?<br>
> > > > ><br>
> > > > > I considered that, but I think that we might as well not make<br>
> > > > > the<br>
> > > > > situation worse for users who do enable trapping math for<br>
> > > > > 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.<br>
> > > > where<br>
> > > > 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<br>
> > > > idea<br>
> > > > (based on when the SIGFPE is delivered) of where the problem<br>
> > > > 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<br>
> > > > > could<br>
> > > > > trap when exceptions are enabled as readnone (including some<br>
> > > > > which<br>
> > > > > never set errno), and if we ignore -ftrapping-math<br>
> > > > > completely,<br>
> > > > > we'll<br>
> > > > > provide no way of indicating that those functions have side<br>
> > > > > effects<br>
> > > > > when we're interested in floating point exceptions.<br>
> > > > ><br>
> > > > ><br>
> > > > > Providing -ftrapping-math as an option is deceptive at best<br>
> > > > > because<br>
> > > > > clang will break code which expects us to honor<br>
> > > > > -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<br>
> > > > 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>
> > ><br>
> > > --<br>
> > > Hal Finkel<br>
> > > Assistant Computational Scientist<br>
> > > Leadership Computing Facility<br>
> > > Argonne National Laboratory<br>
> > ><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > <a href="mailto:cfe-commits@cs.uiuc.edu">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>
<span class=""><font color="#888888">><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">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>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory</font></span></blockquote></div><br></div></div>