RE: [PATCH v3 05/16] MIPS: math-emu: <MAX|MAXA|MIN|MINA>.<D|S>: Fix quiet NaN propagation

[Aleksandar Markovic]

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