<div dir="ltr">LGTM.<div><br></div><div>-Eli</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 13, 2013 at 10:05 AM, 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">Ping.<br>
<div class="HOEnZb"><div class="h5"><br>
Thanks again,<br>
Hal<br>
<br>
----- Original Message -----<br>
> Eli,<br>
><br>
> I've revised the patch, adding back for 'e' flags where POSIX says<br>
> that errno might be modified (even if the Linux man pages claim<br>
> otherwise); with the following exceptions:<br>
><br>
> For ceil, floor, nearbyint, rint - The Linux man page explains that<br>
> the conditions under which the documented errno values might be set<br>
> are impossible on all known IEEE systems [this does not represent a<br>
> functionality change from the current definitions, I'm just noting<br>
> it here for completeness].<br>
><br>
> Note that this changes the current definitions for lrint and fma<br>
> (unfortunately). The Linux man page documents that these don't set<br>
> errno, but the POSIX standard says that they should.<br>
><br>
> Thanks again,<br>
> Hal<br>
><br>
> ----- Original Message -----<br>
> > Eli,<br>
> ><br>
> > Here's the third (and, for now, final) part: adding new LIBBUILTIN<br>
> > definitions to match the existing libm __builtin_* definitions.<br>
> ><br>
> > As it turns out, I think that you were right: separating out the<br>
> > changes to the existing definitions does make it easier to see what<br>
> > is changing :)<br>
> ><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<br>
> > > > 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,<br>
> > > 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>
> > > > > ><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<br>
> > > > > 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 <<br>
> > > > > > <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 3:32 PM, Hal Finkel <<br>
> > > > > > > <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<br>
> > > > > > > > function<br>
> > > > > > > > definitions, and improves the way in which<br>
> > > > > > > > -ftrapping-math<br>
> > > > > > > > is<br>
> > > > > > > > handled in a complementary way. Specifically, this<br>
> > > > > > > > patch:<br>
> > > > > > > ><br>
> > > > > > > > 1. The libm LIBBUILTIN definitions are synchronized<br>
> > > > > > > > with<br>
> > > > > > > > the<br>
> > > > > > > > __builtin_* definitions for libm functions. We<br>
> > > > > > > > currently<br>
> > > > > > > > have<br>
> > > > > > > > __builtin_* definitions for many more functions than<br>
> > > > > > > > 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<br>
> > > > > > > > const<br>
> > > > > > > > so<br>
> > > > > > > > long<br>
> > > > > > > > as<br>
> > > > > > > > trapping math is disabled. I've added a TrappingMath<br>
> > > > > > > > lang<br>
> > > > > > > > option<br>
> > > > > > > > to<br>
> > > > > > > > track this.<br>
> > > > > > > ><br>
> > > > > > > > 3. Disables -ftrapping-math by default. This is<br>
> > > > > > > > currently<br>
> > > > > > > > enabled<br>
> > > > > > > > by<br>
> > > > > > > > default (following gcc), but this is silly. No system<br>
> > > > > > > > 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<br>
> > > > > > > > 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<br>
> > > > > > > > (and<br>
> > > > > > > > enable<br>
> > > > > > > > better code generation around the functions for which<br>
> > > > > > > > we<br>
> > > > > > > > have<br>
> > > > > > > > LIBBUILTIN definitions).<br>
> > > > > > > ><br>
> > > > > > > > 4. Updates the CodeGen/libcall-declarations.c test<br>
> > > > > > > > case.<br>
> > > > > > > ><br>
> > > > > > > > As a side effect, the existing libm LIBBUILTIN<br>
> > > > > > > > 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<br>
> > > > > > > > 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<br>
> > > > > > > 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<br>
> > > > > > > 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<br>
> > > > > > > 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<br>
> > > > > > > you<br>
> > > > > > > were<br>
> > > > > > > changing a bunch of existing ones, but if the balance is<br>
> > > > > > > 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,<br>
> > > > > > I'll<br>
> > > > > > go<br>
> > > > > > over<br>
> > > > > > them again, and try to make that more clear in the<br>
> > > > > > splitting.<br>
> > > > > ><br>
> > > > > ><br>
> > > > > > ><br>
> > > > > > ><br>
> > > > > > ><br>
> > > > > > > ><br>
> > > > > > > > Your changes could be substantially simplified if we<br>
> > > > > > > > don't<br>
> > > > > > > > allow<br>
> > > > > > > > enabling -ftrapping-math; as you've noted, we don't<br>
> > > > > > > > honor<br>
> > > > > > > > it<br>
> > > > > > > > anyway.<br>
> > > > > > > > What do you think?<br>
> > > > > > ><br>
> > > > > > > I considered that, but I think that we might as well not<br>
> > > > > > > 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 |<br>
> > > > > > 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<br>
> > > > > > better<br>
> > > > > > idea<br>
> > > > > > (based on when the SIGFPE is delivered) of where the<br>
> > > > > > 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<br>
> > > > > > > that<br>
> > > > > > > could<br>
> > > > > > > trap when exceptions are enabled as readnone (including<br>
> > > > > > > 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<br>
> > > > > > > side<br>
> > > > > > > effects<br>
> > > > > > > when we're interested in floating point exceptions.<br>
> > > > > > ><br>
> > > > > > ><br>
> > > > > > > Providing -ftrapping-math as an option is deceptive at<br>
> > > > > > > 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<br>
> > > > > > error?)<br>
> > > > > > 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>
> > ><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<br>
> ><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<br>
><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<br>
</div></div></blockquote></div><br></div>