<div dir="ltr">On Fri, Sep 6, 2013 at 3:53 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">----- 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> > 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 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 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 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 I'm<br>
> > aware actually traps on floating-point exceptions by default (some<br>
> > system-specific mechanism is necessary in order to enable SIGFPE<br>
> > generation). In addition, in Clang, we currently don't support 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 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>
> FWIW, I think that reviewing the Builtins.def changes means, in<br>
> practice (either trusting me, or) going through the documentation 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 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 say,<br>
> it might not be as useful as I'd hope.<br>
<br>
</div></div>There may have been a few changes to the existing ones, I'll go over them again, and try to make that more clear in the splitting.<br>
<div class="im"><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>
</div>If I have an application that is producing NaNs, Inf, etc. where that 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() & ~(_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 (based on when the SIGFPE is delivered) of where the problem is.<br></blockquote><div><br></div><div>Ah, interesting.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><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, 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>
</div>Fair enough. I'll leave that out for now, and we can always revisit it later if someone cares.<br>
<br>
As a separate patch, we could issue a warning (or an error?) if -ftrapping-math is provided?<br>
<div class="HOEnZb"><div class="h5"></div></div></blockquote></div><br></div><div class="gmail_extra">Yes, that's a good idea.</div><div class="gmail_extra"><br></div><div class="gmail_extra">-Eli</div></div>