From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66569 invoked by alias); 7 May 2019 19:38:56 -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 66040 invoked by uid 89); 7 May 2019 19:38:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Spam-Relays-External:209.85.208.68, H*RU:209.85.208.68, H*c:alternative X-HELO: mail-ed1-f68.google.com Received: from mail-ed1-f68.google.com (HELO mail-ed1-f68.google.com) (209.85.208.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 May 2019 19:38:54 +0000 Received: by mail-ed1-f68.google.com with SMTP id n17so19853071edb.0 for ; Tue, 07 May 2019 12:38:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ARhRg+kuBQT2FZRsbCDDvVTd90Ta6E9T2clrNU7Esbw=; b=Ko+ksE8fbFw2EspdLomfMi9ZAqT8Mi51ZudauUPXL754qfYpCslM+4k1144SZB+xOW 4ABFUgxMq95WOlaUZXFcENj4Lw4nWyftAFxTXGLcZHT1MbM7YgM8EG0w0E8+Hc7jCfiM SAuFHLHI3n9i8murAs2uC/hQFgdTijfoIBI5z17lS0jWY1A57FFJoo8wR1DPSfWn9Pld oixoDdTUl435/66nPVyHUO2KLR5kmK4uv1dF8+2bKVuPOsU+N8jknmh5pmwMLXV0Obud i2P++ZVosvZcEIPUAg+fC5uM1COr5+5EWZ9q0uX1SA5ogkzwk/Ivc7JkJ+jANOV4ck1K dBcA== MIME-Version: 1.0 References: In-Reply-To: From: Tejas Joshi Date: Tue, 07 May 2019 19:38:00 -0000 Message-ID: Subject: Re: About GSOC. To: gcc@gcc.gnu.org Cc: joseph@codesourcery.com, Martin Jambor Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00060.txt.bz2 Hello. Thanks for your inputs. 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). Hello. As per my understanding, 3.5 would be represented in GCC as follows : r->uexp = 2 and r->sig[2] = 1110000....00 in binary 64 bit. (first 2 bits being 3 and following 1000....0 being 0.5, which later only happens for halfway cases) So, if we clear out the significand part and the immediate bit to the right which represent 0.5, the entire significand would become 0 due to bit-wise ANDing. > + tempsig[w] &= (((unsigned long)1 << ((n % HOST_BITS_PER_LONG) - 1)) - > 1); > That is what the following line intend to do. The clearing part would change the significand, that's why significand was copied to a temporary array for checking. This logic is inspired by the clear_significand_below function. Or isn't this the way it was meant to be implemented? Also, why unsigned long sig[SIGSZ] has to be an array? If n % HOST_BITS_PER_LONG is 0, this code would shift by -1, which isn't > Yes, the condition checking is necessary here. I will incorporate the changes and find a way to check if the number is even or odd bit-wise and add test cases in the test suite as soon as possible. Thanks. Regards, -Tejas On Tue, 7 May 2019 at 22:47, Joseph Myers wrote: > 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 >