Re: [PATCH] Add type arg to TARGET_LIBC_HAS_FUNCTION

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Sep 29, 2020 at 2:18 PM Tom de Vries <tdevries@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 <tdevries@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