* [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).