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

[Alex Bennée]

Aleksandar Markovic <address@hidden> writes:

>> From: Alex Bennée <address@hidden>
>> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>>
>> Mateja Marjanovic <address@hidden> writes:
>>
>> > From: Mateja Marjanovic <address@hidden>
>> >
>> > Wrong type of NaN was generated by maddf and msubf insturctions
>> > when the arguments were inf, zero, nan or zero, inf, nan
>> > respectively.
>> >
>> > Signed-off-by: Mateja Marjanovic <address@hidden>
>> > ---
>> >  fpu/softfloat-specialize.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> > index 16c0bcb..647bfbc 100644
>> > --- a/fpu/softfloat-specialize.h
>> > +++ b/fpu/softfloat-specialize.h
>> > @@ -500,7 +500,7 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass 
>> > b_cls, FloatClass c_cls,
>> >       */
>> >      if (infzero) {
>> >          float_raise(float_flag_invalid, status);
>> > -        return 3;
>> > +        return 2;
>>
>> Hi,
>>
>> This changes the behaviour documented above which says:
>>
>>     /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
>>      * the default NaN
>>      */
>>
>> So if the behaviour is incorrect please update the comment as well.
>> Bonus points for a reference to the canonical reference document that
>> describes this.
>>
>
> Alex,
>
> I tested this case (0*inf+nan fused multiply-add/multiply-subtract) on a MIPS
> hardware system, and this patch is right - after the patch QEMU and hardware
> (MIPS64R6 board) behaviors match. The comment you mentioned probably
> just reflects the code, it looks not to be the reference source of 
> information.
> If the patch is accepted, that comments must be changed in the same
> patch.

I'm perfectly happy to take your word the fix is fine for MIPS - as you
say this is part of the rich tapestry of IMPDEF variations on NaN
handling ;-)

I did have a brief browse through the MIPS Architecture for Programmers
(Document Number: MD00083) but couldn't find a decent line about the NaN
propagation but if it's somewhere else it would be worth mentioning in
the comment.

>
> This is a MIPS-only-specific change (under "#if defined (TARGET_MIPS)"), and,
> from your point of view, could this be included in MIPS pull request (if the 
> validity
> of the patch is confirmed)?

I'd be happy for you to included it (with the matching comment change)
in your next PR:

Acked-by: Alex Bennée <address@hidden>

>
> Sincerely,
> Aleksandar


--
Alex Bennée