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

[Aleksandar Markovic]

> From: Alex Bennée <address@hidden>
> 
> > Aleksandar Markovic <address@hidden> writes:
> 
> >> From: Peter Maydell <address@hidden>
> >> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
> >>
> >> On Mon, 11 Mar 2019 at 11:52, Aleksandar Markovic
> >> <address@hidden> wrote:
> >> >
> >> > > From: Mateja Marjanovic <address@hidden>
> >> > > 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.

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

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