Hi, James,
I appreciate your thorough and expeditious review.
>
> ________________________________________
> From: James Hogan
> Sent: Friday, July 21, 2017 7:45 AM
> To: Aleksandar Markovic
> Cc: linux-mips@xxxxxxxxxxxxxx; Aleksandar Markovic; Miodrag Dinic; Goran Ferenc; Douglas Leung; linux-kernel@xxxxxxxxxxxxxxx; Paul Burton; Petar Jovanovic; Raghu Gandham; Ralf Baechle
> Subject: Re: [PATCH v3 05/16] MIPS: math-emu: <MAX|MAXA|MIN|MINA>.<D|S>: Fix quiet NaN propagation
>
> On Fri, Jul 21, 2017 at 04:09:03PM +0200, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <aleksandar.markovic@xxxxxxxxxx>
> >
> > Fix the value returned by <MAX|MAXA|MIN|MINA>.<D|S>, if both inputs
> > are quiet NaNs. The specifications of <MAX|MAXA|MIN|MINA>.<D|S> state
> > that the returned value in such cases should be the quiet NaN
> > contained in register fs.
> >
> > The relevant example:
> >
> > MAX.S fd,fs,ft:
> > If fs contains qNaN1, and ft contains qNaN2, fd is going to contain
> > qNaN1 (without this patch, it used to contain qNaN2).
> >
>
> Consider adding:
>
> Fixes: a79f5f9ba508 ("MIPS: math-emu: Add support for the MIPS R6 MAX{, A} FPU instruction")
> Fixes: 4e9561b20e2f ("MIPS: math-emu: Add support for the MIPS R6 MIN{, A} FPU instruction")
>
Will add in v4 (for all MIN/MAX/MINA/MAXa patches).
> > Signed-off-by: Miodrag Dinic <miodrag.dinic@xxxxxxxxxx>
> > Signed-off-by: Goran Ferenc <goran.ferenc@xxxxxxxxxx>
> > Signed-off-by: Aleksandar Markovic <aleksandar.markovic@xxxxxxxxxx>
>
> Consider adding:
>
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.3+
Will add on v4 (for all MIN/MAX/MINA/MAXA patches).
> > ---
> > arch/mips/math-emu/dp_fmax.c | 8 ++++++--
> > arch/mips/math-emu/dp_fmin.c | 8 ++++++--
> > arch/mips/math-emu/sp_fmax.c | 8 ++++++--
> > arch/mips/math-emu/sp_fmin.c | 8 ++++++--
> > 4 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/mips/math-emu/dp_fmax.c b/arch/mips/math-emu/dp_fmax.c
> > index fd71b8d..567fc33 100644
> > --- a/arch/mips/math-emu/dp_fmax.c
> > +++ b/arch/mips/math-emu/dp_fmax.c
> > @@ -47,6 +47,9 @@ union ieee754dp ieee754dp_fmax(union ieee754dp x, union ieee754dp y)
> > case CLPAIR(IEEE754_CLASS_SNAN, IEEE754_CLASS_INF):
> > return ieee754dp_nanxcpt(x);
> >
> > + case CLPAIR(IEEE754_CLASS_QNAN, IEEE754_CLASS_QNAN):
> > + return x;
>
> couldn't the above...
>
> > +
> > /* numbers are preferred to NaNs */
> > case CLPAIR(IEEE754_CLASS_ZERO, IEEE754_CLASS_QNAN):
> > case CLPAIR(IEEE754_CLASS_NORM, IEEE754_CLASS_QNAN):
> > @@ -54,7 +57,6 @@ union ieee754dp ieee754dp_fmax(union ieee754dp x, union ieee754dp y)
>
> ... go somewhere around here and fall through to the existing return x
> case?
>
It could, but at the expense of code clarity and/or logical grouping of special cases,
which after this patch looks like:
. . .
|
case of both inputs qNaN
|
case of only x input qNaN
|
case of only y input qNaN
|
. . .
If you agree, I suggest keeping the code the same as currently proposed in
this patch, except that the following comments should be added in appropriate
places:
/*
* Quiet NaN handling
*/
/* The case of both inputs quiet NaNs */
. . .
/* The cases of exactly one input quiet NaN */
Unfortunately, the code segment for handling of sNaN and infinity inputs do
not follow the organization that I proposed. However, I think that my proposal
for case organization is the superior one - therefore I intend to keep it in v4,
unless you tell me not to do so.
Regards,
Aleksandar
