[PATCH] Trapping math and libfuncs improvements

[Hal Finkel]

----- 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.

> 
> 
> 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?

Thanks again,
Hal

> 
> 
> -Eli

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