From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44536 invoked by alias); 20 May 2019 15:49:41 -0000 Mailing-List: contact gcc-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-owner@gcc.gnu.org Received: (qmail 44527 invoked by uid 89); 20 May 2019 15:49:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=editor, 0.25, 5426, 025 X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 May 2019 15:49:39 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 3956DAFF8; Mon, 20 May 2019 15:49:37 +0000 (UTC) From: Martin Jambor To: Tejas Joshi , gcc@gcc.gnu.org Cc: Subject: Re: About GSOC. In-Reply-To: References: User-Agent: Notmuch/0.28.4 (https://notmuchmail.org) Emacs/26.2 (x86_64-suse-linux-gnu) Date: Mon, 20 May 2019 15:49:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00162.txt.bz2 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