[PATCH] Trapping math and libfuncs improvements [Part 3 - new definitions]
Hal Finkel
hfinkel at anl.gov
Fri Sep 6 23:49:30 PDT 2013
Eli,
I've revised the patch, adding back for 'e' flags where POSIX says that errno might be modified (even if the Linux man pages claim otherwise); with the following exceptions:
For ceil, floor, nearbyint, rint - The Linux man page explains that the conditions under which the documented errno values might be set are impossible on all known IEEE systems [this does not represent a functionality change from the current definitions, I'm just noting it here for completeness].
Note that this changes the current definitions for lrint and fma (unfortunately). The Linux man page documents that these don't set errno, but the POSIX standard says that they should.
Thanks again,
Hal
----- Original Message -----
> Eli,
>
> Here's the third (and, for now, final) part: adding new LIBBUILTIN
> definitions to match the existing libm __builtin_* definitions.
>
> As it turns out, I think that you were right: separating out the
> changes to the existing definitions does make it easier to see what
> is changing :)
>
> 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
>
> _______________________________________________
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: new_and_rev_libbuiltins.patch
Type: text/x-patch
Size: 50565 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130907/63267933/attachment.bin>
More information about the cfe-commits
mailing list