Re: About GSOC.

[Martin Jambor <mjambor@suse.cz>]

Hello Tejas,

On Wed, May 08 2019, Tejas Joshi wrote:
> Hello.
> I can't figure out from the documentation how to add test cases in the
> testsuite and inspect the results. How can I do that? Although, Taking
> the mentioned conditions under consideration, I have made another
> patch, attached.

in addition to the things already pointed out by Joseph, I have the
following comments.  But as Joseph has already pointed out, you should
also test your patch on __float128 types, so please make sure your code
gets invoked and works for something like:

    if (__builtin_roundevenf128 (0x1p64q+0.5) != (0x1p64q))
      link_error (__LINE__);


> diff --git a/gcc/builtins.def b/gcc/builtins.def
> index ef89729fd0c..e1d593a8765 100644
> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -542,6 +542,9 @@ DEF_C99_BUILTIN        (BUILT_IN_RINTL, "rintl", BT_FN_LONGDOUBLE_LONGDOUBLE, AT
>  #define RINT_TYPE(F) BT_FN_##F##_##F
>  DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_RINT, "rint", RINT_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST)
>  #undef RINT_TYPE
> +DEF_EXT_LIB_BUILTIN    (BUILT_IN_ROUNDEVEN, "roundeven", BT_FN_DOUBLE_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
> +DEF_EXT_LIB_BUILTIN    (BUILT_IN_ROUNDEVENF, "roundevenf", BT_FN_FLOAT_FLOAT, ATTR_CONST_NOTHROW_LEAF_LIST)
> +DEF_EXT_LIB_BUILTIN    (BUILT_IN_ROUNDEVENL, "roundevenl", BT_FN_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)

...and for the code to trigger for __builtin_roundevenf128 you have to
define this builtin function too.  The easiest way is to do it in the
same way it is done for BUILTIN_ROUND and many other functions, i.e. use
DEF_EXT_LIB_FLOATN_NX_BUILTINS.


> diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c
> index 06a420601c0..7eafd91e9a2 100644
> --- a/gcc/fold-const-call.c
> +++ b/gcc/fold-const-call.c
> @@ -792,6 +792,14 @@ fold_const_call_ss (real_value *result, combined_fn fn,
>  	}
>        return false;
>  
> +    case CFN_BUILT_IN_ROUNDEVEN:

If you look at other cases in this switch statement, they all use
CASE_CFN_* macros.  These the get expanded to all the type variants of
the builting function, so please do the same thing and replace the case
with

    CASE_CFN_ROUNDEVEN:
    CASE_CFN_ROUNDEVEN_FN:


> +      if (!REAL_VALUE_ISNAN (*arg) || !flag_errno_math)
> +  {

Make sure you set your editor to follow the GNU coding style.  The
horizontal position of the curly braces here is incorrect.  Insisting on
coding style details may sound like excessive nit-picking but in the
long run it makes the sources much more readable.

> +    real_roundeven (result, format, arg);
> +    return true;
> +  }
> +      return false;
> +
>      CASE_CFN_LOGB:
>        return fold_const_logb (result, arg, format);
>  
> @@ -854,6 +862,9 @@ fold_const_call_ss (wide_int *result, combined_fn fn,
>        return fold_const_conversion (result, real_round, arg,
>  				    precision, format);
>  
> +    case CFN_BUILT_IN_ROUNDEVEN:

the comment about CASE_CFN_* macros applies here as well.

> +      return fold_const_conversion (result, real_roundeven, arg, precision, format);
> +
>      CASE_CFN_IRINT:
>      CASE_CFN_LRINT:
>      CASE_CFN_LLRINT:
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index 59cedeafd71..30c409e95bf 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -329,6 +329,7 @@ negate_mathfn_p (combined_fn fn)
>      CASE_CFN_LLROUND:
>      CASE_CFN_LROUND:
>      CASE_CFN_ROUND:
> +    CASE_CFN_ROUNDEVEN:

and CASE_CFN_ROUNDEVEN_FN:

>      CASE_CFN_SIN:
>      CASE_CFN_SINH:
>      CASE_CFN_TAN:
> @@ -13060,6 +13061,8 @@ tree_call_nonnegative_warnv_p (tree type, combined_fn fn, tree arg0, tree arg1,
>      CASE_CFN_RINT_FN:
>      CASE_CFN_ROUND:
>      CASE_CFN_ROUND_FN:
> +    CASE_CFN_ROUNDEVEN:
> +    CASE_CFN_ROUNDEVEN_FN:
>      CASE_CFN_SCALB:
>      CASE_CFN_SCALBLN:
>      CASE_CFN_SCALBN:
> @@ -13583,6 +13586,8 @@ integer_valued_real_call_p (combined_fn fn, tree arg0, tree arg1, int depth)
>      CASE_CFN_RINT_FN:
>      CASE_CFN_ROUND:
>      CASE_CFN_ROUND_FN:
> +    CASE_CFN_ROUNDEVEN:
> +    CASE_CFN_ROUNDEVEN_FN:
>      CASE_CFN_TRUNC:
>      CASE_CFN_TRUNC_FN:
>        return true;
> diff --git a/gcc/real.c b/gcc/real.c
> index f822ae82d61..045dc758048 100644
> --- a/gcc/real.c
> +++ b/gcc/real.c
> @@ -5010,6 +5010,53 @@ real_round (REAL_VALUE_TYPE *r, format_helper fmt,
>      real_convert (r, fmt, r);
>  }
>  
> +/* Return true if integer part of R is even, else return false. */

If the function is supposed to work only on integer values (encoded as
REAL_VALUE_TYPE) then please write that into the function comment.

> +
> +bool
> +is_even (REAL_VALUE_TYPE *r)
> +{
> +  unsigned int n = SIGNIFICAND_BITS - REAL_EXP (r);

Otherwise, if you really wanted to make it work on integer parts of any
real number, this is clearly wrong for any value smaller than 0.5
(i.e. when REAL_EXP (r) is negative).

> +
> +  unsigned long num = ((unsigned long)1 << (n % HOST_BITS_PER_LONG));
> +
> +  if ((r->sig[SIGSZ-1] & num) == 0)
> +    return true;

I don't think you can expect that the digit you are looking for is in
the last element of sig.

> +  return false;
> +}
> +
> +/* Return true if R is halfway between two integers, else return false. */
> +
> +bool
> +is_halfway_below (const REAL_VALUE_TYPE *r)
> +{
> +  if (r->sig[0] != 0 && r->sig[1] != 0)
> +    return false;

If you try this code on the example I gave at the beginning of my email,
you will see that this condition is not met.


> +
> +  unsigned int n = SIGNIFICAND_BITS - REAL_EXP (r);

Joseph has already pointed out that you can and should bail out if
REAL_EXP (r) is negative, because then the number is in the internal
[-0.25, 0.25] or when it is too big because then it is already an
integer.

> +
> +  unsigned long num = ((unsigned long)1 << ((n - 1) % HOST_BITS_PER_LONG));
> +
> +  if (((r->sig[SIGSZ-1] & num) != 0) && ((r->sig[SIGSZ-1] & (num-1)) == 0))
> +    return true;

And he also pointed out this is wrong too, just think how 0x1p64q+0.5,
i.e. 2^64+ 2^(-1) is represented.  It just cannot fit into one 64-bit long.

Nevertheless, you are definitely on the right track and these things can
be confusing at first, so I'm looking forward to the next iteration of
the patch.

Martin