[PATCH] Trapping math and libfuncs improvements

[Eli Friedman]

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.

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


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

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130906/28659c5d/attachment.html>