Re: Aw: Re: Re: [PATCH] FPU IEEE 754 for MIPS r5900

[Richard Sandiford <rdsandiford at googlemail dot com>]

"JÃrgen Urban" <JuergenUrban@gmx.de> writes:
> Hello,
>> "JÃrgen Urban" <JuergenUrban@gmx.de> writes:
>> >> "JÃrgen Urban" <JuergenUrban@gmx.de> writes:
>> >> > I used the SPU code in GCC as example for creating an
>> >> > r5900_single_format structure. The patch is attached to the e-mail. I
>> >> > want to submit this patch.
>> >>
>> >> Thanks.  Are there any real differences though?  E.g. in your version
>> >> you set has_sign_dependent_rounding, but that's not necessary when the
>> >> only rounding mode is towards zero.  has_sign_dependent_rounding says
>> >> whether rounding X vs. -X can give numbers of different magnitude.
>> >> (It was actually because of r5900 that this distinction was added.)
>> >>
>> >> I'm also not sure it makes sense to choose a different NaN encoding
>> >> when NaNs aren't supported anyway.
>> >>
>> >> It would be good if we could reuse spu_single_format directly.
>> >
>> > I don't know what the effect of has_sign_dependent_rounding is.
>>
>> Like I say, it tells GCC whether -X can round to something other than -Y
>> in cases where X would round to Y.  This is true for IEEE when rounding
>> towards +infinity or -infinity, but those modes aren't supported on the
>> R5900.
>>
>> Some transformations are invalid when has_sign_dependent is true.
>> E.g. -(X - Y) is not always equal to Y - X.  We want it to be false
>> when possible, so it looked like the spu_single_format version was right.
>
> The manual says that the rounding differs in the least significant bit,
> so we need to assume that this can happen any time and it also leads to
> a different result when the sign is different.

Just to be clear, do you mean different from IEEE?  That doesn't matter
for defining has_sign_dependent_rounding, which is comparing the R5900
rounding of intermediate result -X vs. the R5900 rounding of intermediate
result X.

> Currently I have problems
> testing it, because the latest GCC which I use (svn r200583) is not able
> to build the Linux kernel, because it has at least 2 bugs. The first is
> the bug 57698 introduced between 17.06. and 26.06. It seems that no GCC
> developer is able to fix it. The second bug affects the dvb_demux.c from
> Linux 2.6.34 when -Os and -mlong is used. This triggers an sanity check
> in do_SUBST in file gcc/combine.c. The first bugs happens on all
> architectures. The second also appears with normal mipsel. There is also
> a bug which annoys me since years, __attribute__((aligned(16))) is not
> working with local variables and doesn't print a warning. This is
> particulary a problem when used in typedefs, because the problem is not
> visible.
> My native ps2sdk doesn't have an implementation of rounding. I don't
> have an environment with a combination of the correct versions of the
> different components for Linux. So I can't test it at the moment.

OK, but the patch does need to be tested before it goes in.
As far as 57698 goes, it would be fine to test with a 17.06 version.

But why do you need to be able to build linux with gcc trunk to test this?
The kernel doesn't use FP anyway.  Can't you just use testcases running
under the kernel you already have?

>> > I also can't test it, because the GCC is already not correctly working
>> > on SPU.
>>
>> Can you give an example?
>
> I wanted to say that I don't know what is correct or not, because the
> GCC is already not handling the other stuff correctly, e.g.:
> inf - inf => 0
> nan - nan => 0
> nan == nan => true (this must be false according to IEEE 754)

Sorry, I was meaning in terms of source code.  I still think these
expressions have no meaning for R5900 floats, where there isn't an
infinity or a nan to begin with.  If the format doesn't have infinity
to begin with, there's no right or wrong answer for "inf - inf"
(__builtin_inff() - __builtin_inff()).  0x7f800000 is not inf,
so what the real FPU does for 0x7f800000 doesn't affect things.

When you construct 0x7f800000 as a finite value it seems to be
handled correctly.  On spu-elf I tried:

float f = 0x1.0p128f - 0x1.0p128f;

and got zero as expected.  Note that the equivalent on x86_64 gives
a warning:

warning: floating constant exceeds range of âfloatâ [-Woverflow]

and produces a NaN.  So it looks like this is working.

> According to the documentation
> http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html: __builting_inf()
> should produce a warning if infinities are not supported. This doesn't
> happen, so I need to assume that GCC thinks that it is supported and
> emulates it, so it should work.

I tried:

  float f = __builtin_inff ();

for spu-elf and got:

  warning: target format does not support infinity [enabled by default]

This doesn't give an error:

  float f = __builtin_inf ();

because doubles do have infinity.  Is that the problem you're seeing?
It's not obvious what should happen there though -- others would know
better than me.  But it shouldn't occur in practice anyway.  The
standard INFINITY macro always uses __builtin_inff, and the code was
written for that assumption:

  /* __builtin_inff is intended to be usable to define INFINITY on all
     targets.  If an infinity is not available, INFINITY expands "to a
     positive constant of type float that overflows at translation
     time", footnote "In this case, using INFINITY will violate the
     constraint in 6.4.4 and thus require a diagnostic." (C99 7.12#4).
     Thus we pedwarn to ensure this constraint violation is
     diagnosed.  */
  if (!MODE_HAS_INFINITIES (TYPE_MODE (type)) && warn)
    pedwarn (loc, 0, "target format does not support infinity");

So using INFINITY should always give the warning.

Directly using the double __builtin_inf instead of the float __builtin_inff
to construct a float inf on a target that doesn't have float infs is a bit
"doctor, it hurts if I do this".  I think we can ignore it for the
initial patch.  Any existing code that converts __builtin_inf to float
isn't going to expect it to be treated as a finite value anyway.

I'm trying to make things simpler here, believe me :-)

Thanks,
Richard