| Message ID | 20250116112536.4117889-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: Fix 0 * Inf + QNaN regression | expand |
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? Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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
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), } FloatInfZeroNaNRule; /* diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c index d0a1e2f3c8a..e0a072b4ebc 100644 --- a/target/i386/tcg/fpu_helper.c +++ b/target/i386/tcg/fpu_helper.c @@ -178,8 +178,11 @@ void cpu_init_fp_statuses(CPUX86State *env) * "Fused-Multiply-ADD (FMA) Numeric Behavior" the NaN handling is * specified -- for 0 * inf + NaN the input NaN is selected, and if * there are multiple input NaNs they are selected in the order a, b, c. + * We also do not raise Invalid for the 0 * inf + (Q)NaN case. */ - set_float_infzeronan_rule(float_infzeronan_dnan_never, &env->sse_status); + set_float_infzeronan_rule(float_infzeronan_dnan_never | + float_infzeronan_suppress_invalid, + &env->sse_status); set_float_3nan_prop_rule(float_3nan_prop_abc, &env->sse_status); /* Default NaN: sign bit set, most significant frac bit set */ set_float_default_nan_pattern(0b11000000, &env->fp_status); diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc index ebde42992fc..4bb341b2f94 100644 --- a/fpu/softfloat-parts.c.inc +++ b/fpu/softfloat-parts.c.inc @@ -126,7 +126,8 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b, float_raise(float_flag_invalid | float_flag_invalid_snan, s); } - if (infzero) { + if (infzero && + !(s->float_infzeronan_rule & float_infzeronan_suppress_invalid)) { /* This is (0 * inf) + NaN or (inf * 0) + NaN */ float_raise(float_flag_invalid | float_flag_invalid_imz, s); } @@ -144,7 +145,7 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b, * Inf * 0 + NaN -- some implementations return the * default NaN here, and some return the input NaN. */ - switch (s->float_infzeronan_rule) { + switch (s->float_infzeronan_rule & ~float_infzeronan_suppress_invalid) { case float_infzeronan_dnan_never: break; case float_infzeronan_dnan_always:
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(-)