Re: [PATCH 1/2] target/i386: Do not raise Invalid for 0 * Inf + QNaN

[Peter Maydell]

On Thu, 16 Jan 2025 at 15:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/16/25 03:25, Peter Maydell wrote:
> > In commit 8adcff4ae7 ("fpu: handle raising Invalid for infzero in
> > pick_nan_muladd") we changed the handling of 0 * Inf + QNaN to always
> > raise the Invalid exception regardless of target architecture.  (This
> > was a change affecting hppa, i386, sh4 and tricore.) However, this
> > was incorrect for i386, which documents in the SDM section 14.5.2
> > that for the 0 * Inf + NaN case that it will only raise the Invalid
> > exception when the input is an SNaN.  (This is permitted by the IEEE
> > 754-2008 specification, which documents that whether we raise Invalid
> > for 0 * Inf + QNaN is implementation defined.)
> >
> > Adjust the softfloat pick_nan_muladd code to allow the target to
> > suppress the raising of Invalid for the inf * zero + NaN case (as an
> > extra flag orthogonal to its choice for when to use the default NaN),
> > and enable that for x86.
> >
> > We do not revert here the behaviour change for hppa, sh4 or tricore:
> >   * The sh4 manual is clear that it should signal Invalid
> >   * The tricore manual is a bit vague but doesn't say it shouldn't
> >   * The hppa manual doesn't talk about fused multiply-add corner
> >     cases at all
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: 8adcff4ae7 (""fpu: handle raising Invalid for infzero in 
> > pick_nan_muladd")
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   include/fpu/softfloat-types.h | 16 +++++++++++++---
> >   target/i386/tcg/fpu_helper.c  |  5 ++++-
> >   fpu/softfloat-parts.c.inc     |  5 +++--
> >   3 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/fpu/softfloat-types.h b/include/fpu/softfloat-types.h
> > index 9d37cdfaa8e..c51b2a5b3de 100644
> > --- a/include/fpu/softfloat-types.h
> > +++ b/include/fpu/softfloat-types.h
> > @@ -278,11 +278,21 @@ typedef enum __attribute__((__packed__)) {
> >       /* No propagation rule specified */
> >       float_infzeronan_none = 0,
> >       /* Result is never the default NaN (so always the input NaN) */
> > -    float_infzeronan_dnan_never,
> > +    float_infzeronan_dnan_never = 1,
> >       /* Result is always the default NaN */
> > -    float_infzeronan_dnan_always,
> > +    float_infzeronan_dnan_always = 2,
> >       /* Result is the default NaN if the input NaN is quiet */
> > -    float_infzeronan_dnan_if_qnan,
> > +    float_infzeronan_dnan_if_qnan = 3,
> > +    /*
> > +     * Don't raise Invalid for 0 * Inf + NaN. Default is to raise.
> > +     * IEEE 754-2008 section 7.2 makes it implementation defined whether
> > +     * 0 * Inf + QNaN raises Invalid or not. Note that 0 * Inf + SNaN will
> > +     * raise the Invalid flag for the SNaN anyway.
> > +     *
> > +     * This is a flag which can be ORed in with any of the above
> > +     * DNaN behaviour options.
> > +     */
> > +    float_infzeronan_suppress_invalid = (1 << 7),
>
> Why 128 and not 4?

I wanted to leave space for adding possible future
values to the dnan options without having to renumber
the suppress_invalid flag. So I put it at the top of
an 8 bit value. But I can use 4 if you prefer.

thanks
-- PMM