From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15832 invoked by alias); 7 May 2019 17:18:01 -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 15815 invoked by uid 89); 7 May 2019 17:18:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=About, herewith X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 May 2019 17:17:59 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=svr-ies-mbx-01.mgc.mentorg.com) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1hO3j2-0005lN-EO from joseph_myers@mentor.com ; Tue, 07 May 2019 10:17:56 -0700 Received: from digraph.polyomino.org.uk (137.202.0.90) by svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Tue, 7 May 2019 18:17:52 +0100 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.90_1) (envelope-from ) id 1hO3iy-0003QD-8j; Tue, 07 May 2019 17:17:52 +0000 Date: Tue, 07 May 2019 17:18:00 -0000 From: Joseph Myers To: Tejas Joshi CC: , Martin Jambor Subject: Re: About GSOC. In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2019-05/txt/msg00057.txt.bz2 On Sat, 4 May 2019, Tejas Joshi wrote: > Hello. > Taking the notes from Joseph under consideration, I have developed a > fairly working patch for roundeven, attached herewith. There are several issues here. One key one is that you haven't added any testcases to the GCC testsuite. I'd expect tests added that test lots of different inputs, for all the float, double and long double types, to verify the results are as expected. That would include various exactly halfway cases - but also cases that are halfway plus or minus 1ulp. Tests would be appropriately conditional on the floating-point formats as needed - testing for IEEE binary128 long double, on configurations that have that type, would help cover certain cases, such as where the integer part exceeds 2^64 but there is still a fractional part. Given tests and confirmation that they have passed in various configurations, it's a lot easier to have confidence in the code - and if possible issues are spotted in the code, they may point the way to missing tests. That is, tests are a key piece of a patch that makes it much easier to review the patch. > I have done bit-wise calculations to check for halfway cases, though > HOST_WIDE_INT is only used to check for even and odd numbers (or is it > necessary to do bit-wise for this too?). Also, why unsigned long Yes, you need to use bit-wise checks for odd and even numbers, because you can have a nonzero fractional part with an integer part that is too big to be represented in HOST_WIDE_INT. With IEEE binary128, you can have 112 bits in the integer part and still have 0.5 as the fractional part. > diff --git a/gcc/real.c b/gcc/real.c > index f822ae82d61..533d471a89b 100644 > --- a/gcc/real.c > +++ b/gcc/real.c > @@ -5010,6 +5010,43 @@ real_round (REAL_VALUE_TYPE *r, format_helper fmt, > real_convert (r, fmt, r); > } > > +bool > +is_halfway_below (const REAL_VALUE_TYPE *r) > +{ > + unsigned long tempsig[SIGSZ]; > + unsigned int n = SIGNIFICAND_BITS - REAL_EXP (r); > + int i, w = n / HOST_BITS_PER_LONG; > + > + for (i = 0; i < SIGSZ; ++i) > + tempsig[i] = r->sig[i]; > + > + for (i = 0; i < w; ++i) > + tempsig[i] = 0; > + > + tempsig[w] &= (((unsigned long)1 << ((n % HOST_BITS_PER_LONG) - 1)) - 1); > + > + if (tempsig[w] == 0) > + return true; > + return false; The logic in this function does not make sense to me. First, it needs a comment above the function defining its exact semantics. Since it lacks a comment, I have to guess based on the name. If it is meant to be testing whether a value is halfway between two integers, there are two things you need to test. You need to test whether the bit with value 0.5 is 0 or 1 (which this function doesn't seem to test) - and you also need to test whether *all* bits below it are zero or not (this function only appears to check bits in a single word, disregarding all the lower words, which is not correct). If n % HOST_BITS_PER_LONG is 0, this code would shift by -1, which isn't valid. You need to allow for cases where either the division between 0.5 and 0.25, or the division between 0.5 and 1, falls exactly at a word boundary in the representation of the significand. It would be a good idea to include various such cases in the tests you add to the testsuite. In any case, there is no need to copy the significand into a temporary array in order to test whether low bits are 0. -- Joseph S. Myers joseph@codesourcery.com