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
jos...@codesourcery.com
