On Tue, Sep 29, 2020 at 2:18 PM Tom de Vries <tdevr...@suse.de> wrote:
>
> On 9/29/20 8:59 AM, Richard Biener wrote:
> > On Mon, Sep 28, 2020 at 7:28 PM Tom de Vries <tdevr...@suse.de> wrote:
> >>
> >> [ was: Re: [Patch][nvptx] return true in libc_has_function for
> >> function_sincos ]
> >>
> >> On 9/26/20 6:47 PM, Tobias Burnus wrote:
> >>> Found when looking at PR97203 (but having no effect there).
> >>>
> >>> The GCC ME optimizes with -O1 (or higher) the
> >>> a = sinf(x)
> >>> b = cosf(x)
> >>> to __builtin_cexpi(x, &a, &b)
> >>> (...i as in internal; like cexp(z) but with with __real__ z == 0)
> >>>
> >>>
> >>> In expand_builtin_cexpi, that is handles as:
> >>> if (optab_handler (sincos_optab, mode) != CODE_FOR_nothing)
> >>> ...
> >>> else if (targetm.libc_has_function (function_sincos))
> >>> ...
> >>> else
> >>> fn = builtin_decl_explicit (BUILT_IN_CEXPF);
> >>>
> >>> And the latter is done. As newlib's cexpf does not know that
> >>> __real__ z == 0, it calculates 'r = expf (__real__ z)' before
> >>> invoking sinf and cosf on __imag__ z.
> >>>
> >>> Thus, it is much faster to call 'sincosf', which also exists
> >>> in newlib.
> >>>
> >>> Solution: Return true for targetm.libc_has_function (function_sincos).
> >>>
> >>>
> >>> NOTE: With -funsafe-math-optimizations (-O0 or higher),
> >>> sinf/cosf and sincosf invoke .sin.approx/.cos/.approx instead of
> >>> doing a library call.
> >>
> >> This version takes care to enable sincos and sincosf, but not sincosl.
> >>
> >> Target hook changes OK for trunk?
> >
> > @@ -9770,7 +9770,7 @@ fold_builtin_sincos (location_t loc,
> > }
> > if (!call)
> > {
> > - if (!targetm.libc_has_function (function_c99_math_complex)
> > + if (!targetm.libc_has_function (function_c99_math_complex, NULL_TREE)
> >
> > why pass NULL_TREE and not 'type' here?
> >
> > || !builtin_decl_implicit_p (fn))
> > return NULL_TREE;
> >
>
> I was trying to do the minimal, sincos-only implementation.
>
> > similar for the builtins.def change for the cases where math functions
> > are affected? I guess it's a bit awkward to make it work there, so OK.
> >
> > bool
> > -darwin_libc_has_function (enum function_class fn_class)
> > +darwin_libc_has_function (enum function_class fn_class, tree type)
> > {
> > - if (fn_class == function_sincos)
> > + if (type != NULL_TREE)
> > + {
> > + switch (fn_class)
> > + {
> > + case function_sincos:
> > + break;
> > + default:
> > + /* Not implemented. */
> > + gcc_unreachable ();
> > + }
> > + }
> >
> > huh. I think special-casing this just for sincos is a bit awkward,
> > esp. ICEing for other queries with a type. Specifically
> >
> > -@deftypefn {Target Hook} bool TARGET_LIBC_HAS_FUNCTION (enum
> > function_class @var{fn_class})
> > +@deftypefn {Target Hook} bool TARGET_LIBC_HAS_FUNCTION (enum
> > function_class @var{fn_class}, tree @var{type})
> > This hook determines whether a function from a class of functions
> > -@var{fn_class} is present in the target C library.
> > +@var{fn_class} is present in the target C library. The @var{type} argument
> > +can be used to distinguish between float, double and long double versions.
> > @end deftypefn
> >
> > This doesn't mention we'll ICE for anything but sincos. A sensible
> > semantics would be that if TYPE is NULL the caller asks for support
> > for all standard (float, double, long double) types while with TYPE
> > non-NULL it can ask for a specific type including for example the
> > new _FloatN, etc. types.
> >
>
> Ack, updated accordingly and retested.
>
> OK for trunk?
OK.
Thanks,
Richard.
> Thanks,
> - Tom
