[GSoC-19] Implementing narrowing functions like fadd

[Tejas Joshi]

Hello.

>> You'll need something different from CASE_MATHFN - these are a different
>> kind of functions that need handling in a different way, because they are
>> parametrized over certain *pairs* of types, rather than over a single
>> type.
> first phase, please just directly test for in the code for
> CFN_BUILT_IN_FADD, CFN_BUILT_IN_FADDL, CFN_BUILT_IN_DADDL and process
> those here to get a prototype working.  When you add support for more

Thanks for the inputs. I have used CFN_BUILT_IN_FADD, etc in this
following patch and function is getting folded.
My question is that how the addition and narrowing should be performed
(is it ok to use do_add for addition?). As functions in real.c does
not operate on any specific precision, just defining the return type
as float would do the trick? Or do I need to make trailing
out-of-precision bits zero? If yes, having functions like
double_to_float would then be useful or such functions already exist
to do the conversion?

Thanks,
Tejas


On Fri, 26 Jul 2019 at 22:49, Martin Jambor <mjambor@suse.cz> wrote:
>
> Hello Tejas,
>
> On Tue, Jul 23 2019, Tejas Joshi wrote:
> > Hi all,
> > I have some doubts regarding narrowing functions. Functions like fadd
> > have different variants like fadd, faddl and daddl. These functions
> > differs from general floating point functions which have float and
> > long double argument types. These functions are defined in
> > builtins.def like floor, floorf and floorl with their respective
> > macros BUILT_IN_FLOOR, BUILT_IN_FLOORF and BUILT_IN_FLOORL. But for
> > fadd, fsub, fmul variants, float is not supposed to be one of the
> > argument types.
> >
> > Also, CASE_MATHFN and CASE_MATHFN_FLOATN cases in builtins.c expand
> > normal, F and L variants which are assigned to respective fcode
> > built_in_function. This makes any function defined in builtins.def to
> > have F and L variants mandatory. How these narrowing functions are
> > supposed to be handled? Do we define another macro expansion like
> > CASE_MATHFN or have a manual case handling in mathfn_built_in_2?
> > Attached patch is what I have tried so far.
>
> They are different so as Joseph pointed out, they need to be handled
> differently, don't feel too constrained by the patterns already in gcc.
> For now you can even just just use directly the constants
> CFN_BUILT_IN_FADD, CFN_BUILT_IN_FADDL, CFN_BUILT_IN_DADDL in the code to
> get a prototype working and then set out to create a macro to wrap this
> in, perhaps even only after you added support for two or more such
> narrowing instructions so that you see how the common parts look like.
>
>
> > diff --git a/gcc/builtins.c b/gcc/builtins.c
> > index 8ceb077b0bf..37c4206ec65 100644
> > --- a/gcc/builtins.c
> > +++ b/gcc/builtins.c
> > @@ -2006,6 +2006,7 @@ mathfn_built_in_2 (tree type, combined_fn fn)
> >      CASE_MATHFN (EXP2)
> >      CASE_MATHFN (EXPM1)
> >      CASE_MATHFN (FABS)
> > +    CASE_MATHFN (FADD)
> >      CASE_MATHFN (FDIM)
> >      CASE_MATHFN_FLOATN (FLOOR)
> >      CASE_MATHFN_FLOATN (FMA)
>
> This does not seem to be necessary for folding, or am I wrong?  If it is
> intended for expansion, I'd drop it from this patch and move to the one
> where it is required.
>
>
>
> > diff --git a/gcc/builtins.def b/gcc/builtins.def
> > index 8bb7027aac7..2e749f038be 100644
> > --- a/gcc/builtins.def
> > +++ b/gcc/builtins.def
> > @@ -352,6 +352,12 @@ DEF_C99_C90RES_BUILTIN (BUILT_IN_FABSL, "fabsl", BT_FN_LONGDOUBLE_LONGDOUBLE, AT
> >  #define FABS_TYPE(F) BT_FN_##F##_##F
> >  DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_FABS, "fabs", FABS_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST)
> >  #undef FABS_TYPE
> > +DEF_EXT_LIB_BUILTIN    (BUILT_IN_FADD, "fadd", BT_FN_FLOAT_DOUBLE_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
> > +DEF_EXT_LIB_BUILTIN    (BUILT_IN_DADDL, "daddl", BT_FN_DOUBLE_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
> > +DEF_EXT_LIB_BUILTIN    (BUILT_IN_FADDL, "faddl", BT_FN_FLOAT_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
> > +#define FADD_TYPE(F) BT_FN_##F##_##F##_##F
> > +DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_FADD, "fadd", FADD_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST)
> > +#undef FADD_TYPE
>
> This results into definitions like BUILT_IN_FADDF32 (what would that
> do?)  and BUILT_IN_FADDF64 or BUILT_IN_FADDF128 (which I don't think we
> want, Joseph?).  Perhaps you can just simply drop this use of
> DEF_EXT_LIB_FLOATN_NX_BUILTINS?
>
> >  DEF_GCC_BUILTIN        (BUILT_IN_FABSD32, "fabsd32", BT_FN_DFLOAT32_DFLOAT32, ATTR_CONST_NOTHROW_LEAF_LIST)
> >  DEF_GCC_BUILTIN        (BUILT_IN_FABSD64, "fabsd64", BT_FN_DFLOAT64_DFLOAT64, ATTR_CONST_NOTHROW_LEAF_LIST)
> >  DEF_GCC_BUILTIN        (BUILT_IN_FABSD128, "fabsd128", BT_FN_DFLOAT128_DFLOAT128, ATTR_CONST_NOTHROW_LEAF_LIST)
> > diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c
> > index d9b546e6803..9f0e5510514 100644
> > --- a/gcc/fold-const-call.c
> > +++ b/gcc/fold-const-call.c
> > @@ -570,6 +570,16 @@ fold_const_nextafter (real_value *result, const real_value *arg0,
> >    return true;
> >  }
> >
> > +static bool
> > +fold_const_fadd (real_value* result, const real_value *arg0,
> > +              const real_value *arg1, const real_format *format)
> > +{
> > +  if (!real_fadd(result, format, arg0, arg1))
> > +    return true;
> > +  else
> > +    return false;
> > +}
> > +
> >  /* Try to evaluate:
> >
> >        *RESULT = ldexp (*ARG0, ARG1)
> > @@ -1366,6 +1376,9 @@ fold_const_call_sss (real_value *result, combined_fn fn,
> >      CASE_CFN_NEXTTOWARD:
> >        return fold_const_nextafter (result, arg0, arg1, format);
> >
> > +    CASE_CFN_FADD:
> > +      return fold_const_fadd (result, arg0, arg1, format);
> > +
>
> As you have pointed out, this does not work, this is not expanded into a
> macro and is just as an unused label for which you get a warning.
> Moreover...
>
> >      default:
> >        return false;
> >      }
> > @@ -1500,6 +1513,20 @@ fold_const_call_1 (combined_fn fn, tree type, tree arg0, tree arg1)
> >         }
> >        return NULL_TREE;
> >      }
> > +  else if (mode != arg0_mode
> > +       && real_cst_p (arg0)
> > +       && real_cst_p (arg1))
> > +    {
> > +      gcc_checking_assert (SCALAR_FLOAT_MODE_P (arg0_mode));
> > +      REAL_VALUE_TYPE result;
> > +      if (arg0_mode == arg1_mode)
> > +     {
> > +       if (fold_const_call_sss (&result, fn, TREE_REAL_CST_PTR (arg0),
> > +                                  TREE_REAL_CST_PTR (arg1),
> > +                                  REAL_MODE_FORMAT (mode)))
>
> ...I don't think you want to stuff handling narrowing functions into
> fold_const_call_sss, that is for operations with matching type.  In the
> first phase, please just directly test for in the code for
> CFN_BUILT_IN_FADD, CFN_BUILT_IN_FADDL, CFN_BUILT_IN_DADDL and process
> those here to get a prototype working.  When you add support for more
> narrowing functions we can then move their processing into a new
> separate function.
>
> At leat that is how I would proceed with folding.
>
> Thanks,
>
> Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fadd-1.diff
Type: text/x-patch
Size: 4554 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20190727/9a27618d/attachment.bin>