----- Original Message ----- > > Looks fine. r190217, thanks!
[I also committed, in r190218, a trivial reordering of the existing definitions.] -Hal > > > -Eli > > > > On Fri, Sep 6, 2013 at 4:10 PM, Hal Finkel < [email protected] > wrote: > > > Eli, > > 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). > > Thanks again, > Hal > > ----- Original Message ----- > > > > 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 > > -- > Hal Finkel > Assistant Computational Scientist > Leadership Computing Facility > Argonne National Laboratory > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
