Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU

["Alex Bennée" <alex.bennee@linaro.org>]

Aleksandar Markovic <amarkovic@wavecomp.com> writes:

>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> > Aleksandar Markovic <amarkovic@wavecomp.com> writes:
>>
>> >> From: Peter Maydell <peter.maydell@linaro.org>
>> >> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>> >>
>> >> On Mon, 11 Mar 2019 at 11:52, Aleksandar Markovic
>> >> <amarkovic@wavecomp.com> wrote:
>> >> >
>> >> > > From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> >> > > Subject: [PATCH] target/mips: Fix minor bug in FPU
>> >> > >
>> >> > > Wrong type of NaN was generated by maddf and msubf insturctions
>> >> > > when the arguments were inf, zero, nan or zero, inf, nan
>> >> > > respectively.
>> >> >
>> >> > I did the applicable tests on both pre-NaN2008 and NaN2008 MIPS hardware,
>> >> > and compared results with QEMu emulations. The underlying reason for this
>> >> > patch is correct, but, as Alex also pointed out, it needs some improvements.
>> >> > However, the softfreeze being so close, I am going to amend the patch while
>> >> > creating the pull request. No respin needed. All in all:
>> >>
>> >> Since this is a bug fix, there is no requirement that it goes in
>> >> before softfreeze, FWIW -- pretty much any bug fix is OK for
>> >> rc1, and a focused bugfix like this one that doesn't affect
>> >> other guest architectures would be ok in rc2 as well.
>> >>
>> >
>> > Thanks, Peter, for the clarification!
>> >
>> > In this case, I think, the most natural would be to wait for the
>> > submitter to submit the v2.
>> >
>> > The reference to the documentation that Alex rightfully asked for
>> > is this:
>> >
>> > "MIPS Architecture for Programmers Volume IV-j: The MIPS32
>> > SIMD Architecture Module", Revision 1.12, February 3, 2016
>> >
>> > The key sentence is on page 53:
>> >
>> >    "If the destination format is floating-point, all NaN propagating
>> >    operations with one NaN operand produce a NaN with the
>> >    payload of the input NaN."
>>
>> Hmm doesn't that fail for:
>>
>>   Inf * Zero + !NaN?
>>
>> Should the check be:
>>
>>     if (infzero) {
>>         float_raise(float_flag_invalid, status);
>>         if (is_nan(c_cls)) {
>>             return 2;
>>         }
>>         return 3;
>>     }
>>
>> ?
>>
>> If either a or b is a NaN we fall through to the NaN selection rules
>> bellow preferring SNaN over QNaN.
>>
>
> Alex,
>
> I'll doublecheck, but I think "infzero" here is a misnomer.
>
> The variable/argument "infzero" actually denotes the cases:
>
>    - "Inf * Zero +/- NaN"
>    - "Inf * Zero +/- NaN"
>    - "Zero * Inf +/- NaN"
>    - "Zero * Inf +/- NaN"
>
> "Inf * Zero +/- !NaN (let's say, normal fp)" is handled
> somewhere else.

Ahh my mistake, infzero only cares about a and b:

    bool inf_zero = ((1 << a.cls) | (1 << b.cls)) ==
                    ((1 << float_class_inf) | (1 << float_class_zero));

but is wrapped in:

    if (is_nan(a.cls) || is_nan(b.cls) || is_nan(c.cls)) {
        return pick_nan_muladd(a, b, c, inf_zero, s);
    }

Which when infzero is true implies c has to be the NaN...

>
> Therefore, "infzero" should be rather called "infzeronan".
> This is from what I remember, but I will reanalyse the
> relevant softfloat code one more time.

No need. I overthought it ;-)

>
> Regards,
> Aleksandar
>
>> >
>> > There are other details in surrounding paragraphs. The document
>> > is for SIMD (MSA) ASE, but both the SIMD and FPU NaN2008
>> > floating point instruction should follow the same rules. I could't
>> > find a separate document on FPU instructions.
>> >
>> > The problem with the current implementation of 0*inf+nan is that
>> > it returns the default NaN (all zeroes in the payload segment),
>> > whereas the documentation says the payload should be from
>> > the input NaN. The behavior specified in the documentation is
>> > confirmed also with tests on hardware.
>> >
>> > Sincerely,
>> > Aleksandar
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée