* [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
@ 2019-03-07 17:10 Mateja Marjanovic
2019-03-07 17:10 ` Mateja Marjanovic
2019-03-11 11:52 ` Aleksandar Markovic
0 siblings, 2 replies; 12+ messages in thread
From: Mateja Marjanovic @ 2019-03-07 17:10 UTC (permalink / raw)
To: qemu-devel; +Cc: aurelien, amarkovic, arikalo, peter.maydell, alex.bennee
From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
Wrong type of NaN was generated by maddf and msubf insturctions
when the arguments were inf, zero, nan or zero, inf, nan
respectively.
Mateja Marjanovic (1):
target/mips: Fix minor bug in FPU
fpu/softfloat-specialize.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-07 17:10 [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU Mateja Marjanovic
@ 2019-03-07 17:10 ` Mateja Marjanovic
2019-03-07 17:32 ` Alex Bennée
2019-03-11 11:52 ` Aleksandar Markovic
1 sibling, 1 reply; 12+ messages in thread
From: Mateja Marjanovic @ 2019-03-07 17:10 UTC (permalink / raw)
To: qemu-devel; +Cc: aurelien, amarkovic, arikalo, peter.maydell, alex.bennee
From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
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 <mateja.marjanovic@rt-rk.com>
---
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;
}
if (snan_bit_is_one(status)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-07 17:10 ` Mateja Marjanovic
@ 2019-03-07 17:32 ` Alex Bennée
2019-03-07 17:43 ` Aleksandar Markovic
0 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2019-03-07 17:32 UTC (permalink / raw)
To: Mateja Marjanovic; +Cc: qemu-devel, aurelien, amarkovic, arikalo, peter.maydell
Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> 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 <mateja.marjanovic@rt-rk.com>
> ---
> 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 Bennée
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-07 17:32 ` Alex Bennée
@ 2019-03-07 17:43 ` Aleksandar Markovic
2019-03-07 18:25 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Aleksandar Markovic @ 2019-03-07 17:43 UTC (permalink / raw)
To: Alex Bennée, Mateja Marjanovic
Cc: qemu-devel@nongnu.org, aurelien@aurel32.net, Aleksandar Rikalo,
peter.maydell@linaro.org
> From: Alex Bennée <alex.bennee@linaro.org>
> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>
> Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:
>
> > From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> >
> > 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 <mateja.marjanovic@rt-rk.com>
> > ---
> > 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.
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)?
Sincerely,
Aleksandar
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-07 17:43 ` Aleksandar Markovic
@ 2019-03-07 18:25 ` Alex Bennée
0 siblings, 0 replies; 12+ messages in thread
From: Alex Bennée @ 2019-03-07 18:25 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Mateja Marjanovic, qemu-devel@nongnu.org, aurelien@aurel32.net,
Aleksandar Rikalo, peter.maydell@linaro.org
Aleksandar Markovic <amarkovic@wavecomp.com> writes:
>> From: Alex Bennée <alex.bennee@linaro.org>
>> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>>
>> Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:
>>
>> > From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>> >
>> > 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 <mateja.marjanovic@rt-rk.com>
>> > ---
>> > 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 <alex.bennee@linaro.org>
>
> Sincerely,
> Aleksandar
--
Alex Bennée
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-07 17:10 [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU Mateja Marjanovic
2019-03-07 17:10 ` Mateja Marjanovic
@ 2019-03-11 11:52 ` Aleksandar Markovic
2019-03-11 12:50 ` Peter Maydell
1 sibling, 1 reply; 12+ messages in thread
From: Aleksandar Markovic @ 2019-03-11 11:52 UTC (permalink / raw)
To: Mateja Marjanovic, qemu-devel@nongnu.org
Cc: aurelien@aurel32.net, Aleksandar Rikalo, peter.maydell@linaro.org,
alex.bennee@linaro.org
> 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:
Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 11:52 ` Aleksandar Markovic
@ 2019-03-11 12:50 ` Peter Maydell
2019-03-11 13:58 ` Aleksandar Markovic
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-03-11 12:50 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Mateja Marjanovic, qemu-devel@nongnu.org, aurelien@aurel32.net,
Aleksandar Rikalo, alex.bennee@linaro.org
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
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 12:50 ` Peter Maydell
@ 2019-03-11 13:58 ` Aleksandar Markovic
2019-03-11 14:18 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Aleksandar Markovic @ 2019-03-11 13:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Mateja Marjanovic, qemu-devel@nongnu.org, aurelien@aurel32.net,
Aleksandar Rikalo, alex.bennee@linaro.org
> 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."
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 13:58 ` Aleksandar Markovic
@ 2019-03-11 14:18 ` Alex Bennée
2019-03-11 14:35 ` Aleksandar Markovic
0 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2019-03-11 14:18 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Peter Maydell, Mateja Marjanovic, qemu-devel@nongnu.org,
aurelien@aurel32.net, Aleksandar Rikalo
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.
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 14:18 ` Alex Bennée
@ 2019-03-11 14:35 ` Aleksandar Markovic
2019-03-11 14:48 ` Peter Maydell
2019-03-11 15:18 ` Alex Bennée
0 siblings, 2 replies; 12+ messages in thread
From: Aleksandar Markovic @ 2019-03-11 14:35 UTC (permalink / raw)
To: Alex Bennée
Cc: Peter Maydell, Mateja Marjanovic, qemu-devel@nongnu.org,
aurelien@aurel32.net, Aleksandar Rikalo
> 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.
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 14:35 ` Aleksandar Markovic
@ 2019-03-11 14:48 ` Peter Maydell
2019-03-11 15:18 ` Alex Bennée
1 sibling, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2019-03-11 14:48 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Alex Bennée, Mateja Marjanovic, qemu-devel@nongnu.org,
aurelien@aurel32.net, Aleksandar Rikalo
On Mon, 11 Mar 2019 at 14:35, Aleksandar Markovic
<amarkovic@wavecomp.com> wrote:
> 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.
Yes; we don't ask the target-dependent code to pick a NaN
in the case of 0 * inf + not-a-NaN because the IEEE spec
entirely defines the behaviour there (you get the default
NaN and Invalid is set), so that case is handled in the
generic softfloat code. (The flag is called 'infzero'
probably because in the calling code it indicates all
the inf * zero cases, not just the info * zero + nan ones).
It's specifically the (0,inf,qnan) and (inf,0,qnan) cases
where the spec allows the implementation to decide whether
they raise Invalid or not (and what qnan to return if they
do), so this is why we pass that flag through to the
pick-a-NaN routine.
It looks like this part of the MIPS-specific code was
incorrectly copied from the Arm implementation (which does
return the default NaN here).
As well as what the right NaN value should be, the other
question for this function for the (0, inf, qnan) case is
"should we raise Invalid?" -- what does the MIPS spec
require here?
thanks
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
2019-03-11 14:35 ` Aleksandar Markovic
2019-03-11 14:48 ` Peter Maydell
@ 2019-03-11 15:18 ` Alex Bennée
1 sibling, 0 replies; 12+ messages in thread
From: Alex Bennée @ 2019-03-11 15:18 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Peter Maydell, Mateja Marjanovic, qemu-devel@nongnu.org,
aurelien@aurel32.net, Aleksandar Rikalo
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-11 15:18 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-07 17:10 [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU Mateja Marjanovic
2019-03-07 17:10 ` Mateja Marjanovic
2019-03-07 17:32 ` Alex Bennée
2019-03-07 17:43 ` Aleksandar Markovic
2019-03-07 18:25 ` Alex Bennée
2019-03-11 11:52 ` Aleksandar Markovic
2019-03-11 12:50 ` Peter Maydell
2019-03-11 13:58 ` Aleksandar Markovic
2019-03-11 14:18 ` Alex Bennée
2019-03-11 14:35 ` Aleksandar Markovic
2019-03-11 14:48 ` Peter Maydell
2019-03-11 15:18 ` Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).