[PATCH] Trapping math and libfuncs improvements [Part 1 - missing n]

[Eli Friedman]

Looks fine.

-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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130906/c88b8a51/attachment.html>