On Fri, Sep 6, 2013 at 5:20 PM, Hal Finkel <[email protected]> wrote: > Eli, > > Here's the second part for review: > > 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). >
http://pubs.opengroup.org/onlinepubs/9699919799/functions/atan.html says atan can in fact set errno. -Eli > > Thanks again, > Hal > > ----- Original Message ----- > > ----- Original Message ----- > > > ----- Original Message ----- > > > > > > > > Looks fine. > > > > > > r190217, thanks! > > > > > > [I also committed, in r190218, a trivial reordering of the existing > > > definitions.] > > > > And in r190222, I reordered the existing definitions to match the > > order of the __builtin_* definitions (which is a small change, but > > should make comparing to the new list easier). > > > > -Hal > > > > > > > > -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 > > > > > > > -- > > 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 > > > > -- > 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
