Re: [GSoC-19] Implementing narrowing functions like fadd

[Tejas Joshi <tejasjoshi9673@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7034 bytes --]

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

[-- Attachment #2: fadd-1.diff --]
[-- Type: text/x-patch, Size: 4554 bytes --]

diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
index e5c9e063c48..6bc552fa71a 100644
--- a/gcc/builtin-types.def
+++ b/gcc/builtin-types.def
@@ -387,8 +387,14 @@ DEF_FUNCTION_TYPE_2 (BT_FN_VOID_UINT_PTR,
 		     BT_VOID, BT_UINT, BT_PTR)
 DEF_FUNCTION_TYPE_2 (BT_FN_FLOAT_FLOAT_FLOAT,
 		     BT_FLOAT, BT_FLOAT, BT_FLOAT)
+DEF_FUNCTION_TYPE_2 (BT_FN_FLOAT_DOUBLE_DOUBLE,
+		     BT_FLOAT, BT_DOUBLE, BT_DOUBLE)
+DEF_FUNCTION_TYPE_2 (BT_FN_FLOAT_LONGDOUBLE_LONGDOUBLE,
+		     BT_FLOAT, BT_LONGDOUBLE, BT_LONGDOUBLE)
 DEF_FUNCTION_TYPE_2 (BT_FN_DOUBLE_DOUBLE_DOUBLE,
 		     BT_DOUBLE, BT_DOUBLE, BT_DOUBLE)
+DEF_FUNCTION_TYPE_2 (BT_FN_DOUBLE_LONGDOUBLE_LONGDOUBLE,
+		     BT_DOUBLE, BT_LONGDOUBLE, BT_LONGDOUBLE)
 DEF_FUNCTION_TYPE_2 (BT_FN_LONGDOUBLE_LONGDOUBLE_LONGDOUBLE,
 		     BT_LONGDOUBLE, BT_LONGDOUBLE, BT_LONGDOUBLE)
 DEF_FUNCTION_TYPE_2 (BT_FN_FLOAT16_FLOAT16_FLOAT16,
diff --git a/gcc/builtins.def b/gcc/builtins.def
index 8bb7027aac7..2df616c477e 100644
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -355,6 +355,9 @@ DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_FABS, "fabs", FABS_TYPE, ATTR_CONST_NOT
 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)
+DEF_EXT_LIB_BUILTIN    (BUILT_IN_FADD, "fadd", BT_FN_FLOAT_DOUBLE_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
+DEF_EXT_LIB_BUILTIN    (BUILT_IN_FADDL, "faddl", BT_FN_FLOAT_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
+DEF_EXT_LIB_BUILTIN    (BUILT_IN_DADDL, "daddl", BT_FN_DOUBLE_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
 DEF_C99_BUILTIN        (BUILT_IN_FDIM, "fdim", BT_FN_DOUBLE_DOUBLE_DOUBLE, ATTR_MATHFN_FPROUNDING_ERRNO)
 DEF_C99_BUILTIN        (BUILT_IN_FDIMF, "fdimf", BT_FN_FLOAT_FLOAT_FLOAT, ATTR_MATHFN_FPROUNDING_ERRNO)
 DEF_C99_BUILTIN        (BUILT_IN_FDIML, "fdiml", BT_FN_LONGDOUBLE_LONGDOUBLE_LONGDOUBLE, ATTR_MATHFN_FPROUNDING_ERRNO)
diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c
index d9b546e6803..374fe517bd6 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)
@@ -1644,6 +1654,25 @@ fold_const_call (combined_fn fn, tree type, tree arg0, tree arg1)
     case CFN_FOLD_LEFT_PLUS:
       return fold_const_fold_left (type, arg0, arg1, PLUS_EXPR);
 
+    case CFN_BUILT_IN_FADD:
+    case CFN_BUILT_IN_FADDL:
+    case CFN_BUILT_IN_DADDL:
+      {
+	if (real_cst_p (arg0)
+	    && real_cst_p (arg1))
+	{
+	  machine_mode arg0_mode = TYPE_MODE (TREE_TYPE (arg0));
+	  gcc_checking_assert (SCALAR_FLOAT_MODE_P (arg0_mode));
+	  REAL_VALUE_TYPE result;
+	  machine_mode mode = TYPE_MODE (type);
+	  if (fold_const_fadd (&result, TREE_REAL_CST_PTR (arg0),
+			       TREE_REAL_CST_PTR (arg1),
+			       REAL_MODE_FORMAT (mode)))
+	    return build_real (type, result);
+	}
+      }
+      return NULL_TREE;
+
     default:
       return fold_const_call_1 (fn, type, arg0, arg1);
     }
diff --git a/gcc/real.c b/gcc/real.c
index ab71430709f..444ab2e27fc 100644
--- a/gcc/real.c
+++ b/gcc/real.c
@@ -5093,6 +5093,16 @@ real_roundeven (REAL_VALUE_TYPE *r, format_helper fmt,
     real_round (r, fmt, x);
 }
 
+bool
+real_fadd (REAL_VALUE_TYPE *r, format_helper fmt,
+	   const REAL_VALUE_TYPE *x, const REAL_VALUE_TYPE *y)
+{
+  do_add (r, x, y, 0);
+  if (fmt)
+    real_convert (r, fmt, r);
+  return false;
+}
+
 /* Set the sign of R to the sign of X.  */
 
 void
diff --git a/gcc/real.h b/gcc/real.h
index 76889bff0ea..f9d388f491b 100644
--- a/gcc/real.h
+++ b/gcc/real.h
@@ -510,6 +510,10 @@ extern void real_round (REAL_VALUE_TYPE *, format_helper,
 extern void real_roundeven (REAL_VALUE_TYPE *, format_helper,
       const REAL_VALUE_TYPE *);
 
+/* Narrowing type standard math operations functions.  */
+extern bool real_fadd (REAL_VALUE_TYPE *, format_helper,
+	const REAL_VALUE_TYPE *, const REAL_VALUE_TYPE *);
+
 /* Set the sign of R to the sign of X.  */
 extern void real_copysign (REAL_VALUE_TYPE *, const REAL_VALUE_TYPE *);