[PATCH] Trapping math and libfuncs improvements [Part 2 - fixing atan]

Fri Sep 6 19:29:29 PDT 2013

----- Original Message -----
> 
> On Fri, Sep 6, 2013 at 5:20 PM, Hal Finkel < hfinkel at anl.gov > 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.

Hrmm... well, that's what I get for not double-checking all of the man pages against the standard. Thanks for checking this; I'll double-check all of the others...

 -Hal

> 
> 
> 
> -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 < hfinkel at anl.gov >
> > > > 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 < hfinkel at anl.gov
> > > > > >
> > > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > ----- Original Message -----
> > > > > > 
> > > > > > On Fri, Sep 6, 2013 at 3:32 PM, Hal Finkel <
> > > > > > hfinkel at anl.gov
> > > > > > > 
> > > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > ----- Original Message -----
> > > > > > > 
> > > > > > > On Fri, Sep 6, 2013 at 8:27 AM, Hal Finkel <
> > > > > > > hfinkel at anl.gov
> > > > > > > > 
> > > > > > > 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
> > > cfe-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > 
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory