On Fri, Sep 6, 2013 at 3:53 PM, Hal Finkel <[email protected]> wrote: > ----- Original Message ----- > > > > On Fri, Sep 6, 2013 at 3:32 PM, Hal Finkel < [email protected] > wrote: > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > On Fri, Sep 6, 2013 at 8:27 AM, Hal Finkel < [email protected] > > > > wrote: > > > > > > > > > > > > > > > Ben, Chad, et al., > > > > > > The attached patch cleans up the LIBBUILTIN libm function > > > definitions, and improves the way in which -ftrapping-math is > > > handled in a complementary way. Specifically, this patch: > > > > > > 1. The libm LIBBUILTIN definitions are synchronized with the > > > __builtin_* definitions for libm functions. We currently have > > > __builtin_* definitions for many more functions than those for > > > which > > > we have LIBBUILTIN definitions. This unnecessarily pessimizes code > > > generation around many of the remaining libm functions. > > > > > > 2. Many of these functions can be marked as readnone so long as we > > > can neglect floating point exceptions. I've added a new specifier > > > 'T', which like 'e' for errno, marks the builtin as const so long > > > as > > > trapping math is disabled. I've added a TrappingMath lang option to > > > track this. > > > > > > 3. Disables -ftrapping-math by default. This is currently enabled > > > by > > > default (following gcc), but this is silly. No system of which I'm > > > aware actually traps on floating-point exceptions by default (some > > > system-specific mechanism is necessary in order to enable SIGFPE > > > generation). In addition, in Clang, we currently don't support fenv > > > access. As a result, we can turn this off by default (and enable > > > better code generation around the functions for which we have > > > LIBBUILTIN definitions). > > > > > > 4. Updates the CodeGen/libcall-declarations.c test case. > > > > > > As a side effect, the existing libm LIBBUILTIN definitions that > > > were > > > missing the 'n' (nothrow) specifier now have it. > > > > > > > > > > > > For clarity, please split the patch into four separate patches: > > > reformatting, adding missing 'n' specifiers, the > > > trapping-math-related changes, and adding new LIBBUILTIN > > > definitions > > > (preferably in that order). As it is, the Builtins.def changes are > > > completely unreviewable. > > > > Unfortunately, because almost all of the LIBBUILTIN's in the patch > > are new, I don't think that splitting the patch will help that :( -- > > FWIW, I think that reviewing the Builtins.def changes means, in > > practice (either trusting me, or) going through the documentation on > > each of those functions to make sure that the flags are correct. I > > don't think that splitting the patch makes that easier either. > > > > Nevertheless, I'll split the changes and re-post as separate patches. > > Maybe I'm wrong ;) > > > > > > > > I didn't try to count how many were new... it looked like you were > > changing a bunch of existing ones, but if the balance is as you say, > > it might not be as useful as I'd hope. > > 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. > > > > > > > > > > > > > Your changes could be substantially simplified if we don't allow > > > enabling -ftrapping-math; as you've noted, we don't honor it > > > anyway. > > > What do you think? > > > > I considered that, but I think that we might as well not make the > > situation worse for users who do enable trapping math for debugging > > purposes (as I do myself on occasion). > > > > > > Debugging how, exactly? > > If I have an application that is producing NaNs, Inf, etc. where that is > not expected, I can add something like this: > > #include <fenv.h> > #if defined(__i386__) && defined(__SSE__) > #include <xmmintrin.h> > #endif > > ... > > #if defined(FE_NOMASK_ENV) && !defined(__INTEL_COMPILER) > fesetenv(FE_NOMASK_ENV); > fedisableexcept(/* FE_OVERFLOW | */ FE_UNDERFLOW | FE_INEXACT); > #elif defined(__i386__) && defined(__SSE__) > _MM_SET_EXCEPTION_MASK(_MM_GET_EXCEPTION_MASK() & > ~(_MM_MASK_OVERFLOW|_MM_MASK_INVALID|_MM_MASK_DIV_ZERO)); > #endif > > 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. >
Ah, interesting. > > > > > > > With these changes, we'll mark a lot more libm functions that could > > trap when exceptions are enabled as readnone (including some which > > never set errno), and if we ignore -ftrapping-math completely, we'll > > provide no way of indicating that those functions have side effects > > when we're interested in floating point exceptions. > > > > > > Providing -ftrapping-math as an option is deceptive at best because > > clang will break code which expects us to honor -ftrapping-math, > > even if it doesn't even use any library calls. > > Fair enough. I'll leave that out for now, and we can always revisit it > later if someone cares. > > As a separate patch, we could issue a warning (or an error?) if > -ftrapping-math is provided? > Yes, that's a good idea. -Eli
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
