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