About GSOC.

Joseph Myers joseph@codesourcery.com
Tue May 7 17:18:00 GMT 2019


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



More information about the Gcc mailing list