[PATCH 0/2] target/i386: Fix 0 * Inf + QNaN regression

[PATCH 0/2] target/i386: Fix 0 * Inf + QNaN regression

[Peter Maydell]

This patchset fixes a regression that I introduced in my recent
refactoring of softfloat NaN handling, in commit 8adcff4ae7
("fpu: handle raising Invalid for infzero in pick_nan_muladd").
When I wrote that code I was under the impression that all
architectures would raise Invalid for the "inf * zero + NaN"
case of a fused multiply-add. However, IEEE 754-2008 makes this
impdef for QNaN: an architecture can choose whether to raise
Invalid or not.

For i386, SDM section 14.5.2 documents that for the 0 * Inf + NaN
case that it will only raise the Invalid exception when the input is
a signalling NaN, and so the behaviour change in 8adcff4ae7 that
caused it to raise Invalid also for the QNaN case is wrong.

The first commit here adds a knob to the softfloat code to
allow an architecture to disable the "raise Invalid" that is
the default, and makes x86 set that. The second commit is a
test case for x86 check-tcg that exercises this corner case.

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

The test case also includes a disabled test for a different
x86 fma corner case; this is one that's not a regression. I've
left it in the test case code because it's the justification
for why the test harness has the support for testing fma insns
with FTZ set. I'm working on a fix for that but I don't think
it should be tangled up with fixing this regression.

thanks
-- PMM

Peter Maydell (2):
  target/i386: Do not raise Invalid for 0 * Inf + QNaN
  tests/tcg/x86_64/fma: Test some x86 fused-multiply-add cases

 include/fpu/softfloat-types.h    |  16 ++++-
 target/i386/tcg/fpu_helper.c     |   5 +-
 tests/tcg/x86_64/fma.c           | 109 +++++++++++++++++++++++++++++++
 fpu/softfloat-parts.c.inc        |   5 +-
 tests/tcg/x86_64/Makefile.target |   1 +
 5 files changed, 130 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/x86_64/fma.c

-- 
2.34.1

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

[Peter Maydell]

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),
 } 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:
-- 
2.34.1

[PATCH 2/2] tests/tcg/x86_64/fma: Test some x86 fused-multiply-add cases

[Peter Maydell]

Add a test case which tests some corner case behaviour of
fused-multiply-add on x86:
 * 0 * Inf + SNaN should raise Invalid
 * 0 * Inf + QNaN shouldh not raise Invalid
 * tininess should be detected after rounding

There is also one currently-disabled test case:
 * flush-to-zero should be done after rounding

This is disabled because QEMU's emulation currently does this
incorrectly (and so would fail the test).  The test case is kept in
but disabled, as the justification for why the test running harness
has support for testing both with and without FTZ set.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/tcg/x86_64/fma.c           | 109 +++++++++++++++++++++++++++++++
 tests/tcg/x86_64/Makefile.target |   1 +
 2 files changed, 110 insertions(+)
 create mode 100644 tests/tcg/x86_64/fma.c

diff --git a/tests/tcg/x86_64/fma.c b/tests/tcg/x86_64/fma.c
new file mode 100644
index 00000000000..09c622ebc00
--- /dev/null
+++ b/tests/tcg/x86_64/fma.c
@@ -0,0 +1,109 @@
+/*
+ * Test some fused multiply add corner cases.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <stdio.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <inttypes.h>
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+/*
+ * Perform one "n * m + a" operation using the vfmadd insn and return
+ * the result; on return *mxcsr_p is set to the bottom 6 bits of MXCSR
+ * (the Flag bits). If ftz is true then we set MXCSR.FTZ while doing
+ * the operation.
+ * We print the operation and its results to stdout.
+ */
+static uint64_t do_fmadd(uint64_t n, uint64_t m, uint64_t a,
+                         bool ftz, uint32_t *mxcsr_p)
+{
+    uint64_t r;
+    uint32_t mxcsr = 0;
+    uint32_t ftz_bit = ftz ? (1 << 15) : 0;
+    uint32_t saved_mxcsr = 0;
+
+    asm volatile("stmxcsr %[saved_mxcsr]\n"
+                 "stmxcsr %[mxcsr]\n"
+                 "andl $0xffff7fc0, %[mxcsr]\n"
+                 "orl %[ftz_bit], %[mxcsr]\n"
+                 "ldmxcsr %[mxcsr]\n"
+                 "movq %[a], %%xmm0\n"
+                 "movq %[m], %%xmm1\n"
+                 "movq %[n], %%xmm2\n"
+                 /* xmm0 = xmm0 + xmm2 * xmm1 */
+                 "vfmadd231sd %%xmm1, %%xmm2, %%xmm0\n"
+                 "movq %%xmm0, %[r]\n"
+                 "stmxcsr %[mxcsr]\n"
+                 "ldmxcsr %[saved_mxcsr]\n"
+                 : [r] "=r" (r), [mxcsr] "=m" (mxcsr),
+                   [saved_mxcsr] "=m" (saved_mxcsr)
+                 : [n] "r" (n), [m] "r" (m), [a] "r" (a),
+                   [ftz_bit] "r" (ftz_bit)
+                 : "xmm0", "xmm1", "xmm2");
+    *mxcsr_p = mxcsr & 0x3f;
+    printf("vfmadd132sd 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64
+           " = 0x%" PRIx64 " MXCSR flags 0x%" PRIx32 "\n",
+           n, m, a, r, *mxcsr_p);
+    return r;
+}
+
+typedef struct testdata {
+    /* Input n, m, a */
+    uint64_t n;
+    uint64_t m;
+    uint64_t a;
+    bool ftz;
+    /* Expected result */
+    uint64_t expected_r;
+    /* Expected low 6 bits of MXCSR (the Flag bits) */
+    uint32_t expected_mxcsr;
+} testdata;
+
+static testdata tests[] = {
+    { 0, 0x7ff0000000000000, 0x7ff000000000aaaa, false, /* 0 * Inf + SNaN */
+      0x7ff800000000aaaa, 1 }, /* Should be QNaN and does raise Invalid */
+    { 0, 0x7ff0000000000000, 0x7ff800000000aaaa, false, /* 0 * Inf + QNaN */
+      0x7ff800000000aaaa, 0 }, /* Should be QNaN and does *not* raise Invalid */
+    /*
+     * These inputs give a result which is tiny before rounding but which
+     * becomes non-tiny after rounding. x86 is a "detect tininess after
+     * rounding" architecture, so it should give a non-denormal result and
+     * not set the Underflow flag (only the Precision flag for an inexact
+     * result).
+     */
+    { 0x3fdfffffffffffff, 0x001fffffffffffff, 0x801fffffffffffff, false,
+      0x8010000000000000, 0x20 },
+    /*
+     * Flushing of denormal outputs to zero should also happen after
+     * rounding, so setting FTZ should not affect the result or the flags.
+     * QEMU currently does not emulate this correctly because we do the
+     * flush-to-zero check before rounding, so we incorrectly produce a
+     * zero result and set Underflow as well as Precision.
+     */
+#ifdef ENABLE_FAILING_TESTS
+    { 0x3fdfffffffffffff, 0x001fffffffffffff, 0x801fffffffffffff, true,
+      0x8010000000000000, 0x20 }, /* Enabling FTZ shouldn't change flags */
+#endif
+};
+
+int main(void)
+{
+    bool passed = true;
+    for (int i = 0; i < ARRAY_SIZE(tests); i++) {
+        uint32_t mxcsr;
+        uint64_t r = do_fmadd(tests[i].n, tests[i].m, tests[i].a,
+                              tests[i].ftz, &mxcsr);
+        if (r != tests[i].expected_r) {
+            printf("expected result 0x%" PRIx64 "\n", tests[i].expected_r);
+            passed = false;
+        }
+        if (mxcsr != tests[i].expected_mxcsr) {
+            printf("expected MXCSR flags 0x%x\n", tests[i].expected_mxcsr);
+            passed = false;
+        }
+    }
+    return passed ? 0 : 1;
+}
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index d6dff559c7d..be20fc64e88 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -18,6 +18,7 @@ X86_64_TESTS += adox
 X86_64_TESTS += test-1648
 X86_64_TESTS += test-2175
 X86_64_TESTS += cross-modifying-code
+X86_64_TESTS += fma
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)
-- 
2.34.1

Re: [PATCH 2/2] tests/tcg/x86_64/fma: Test some x86 fused-multiply-add cases

[Richard Henderson]

On 1/16/25 03:25, Peter Maydell wrote:
> Add a test case which tests some corner case behaviour of
> fused-multiply-add on x86:
>   * 0 * Inf + SNaN should raise Invalid
>   * 0 * Inf + QNaN shouldh not raise Invalid
>   * tininess should be detected after rounding
> 
> There is also one currently-disabled test case:
>   * flush-to-zero should be done after rounding
> 
> This is disabled because QEMU's emulation currently does this
> incorrectly (and so would fail the test).  The test case is kept in
> but disabled, as the justification for why the test running harness
> has support for testing both with and without FTZ set.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   tests/tcg/x86_64/fma.c           | 109 +++++++++++++++++++++++++++++++
>   tests/tcg/x86_64/Makefile.target |   1 +
>   2 files changed, 110 insertions(+)
>   create mode 100644 tests/tcg/x86_64/fma.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Richard Henderson]

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~

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

Re: [PATCH 0/2] target/i386: Fix 0 * Inf + QNaN regression

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH 0/2] target/i386: Fix 0 * Inf + QNaN regression

[Peter Maydell]

On Fri, 24 Jan 2025 at 17:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Queued, thanks.

Thanks; do you plan to send a pullreq with these in soon?
I ask because the Arm FEAT_AFP set is now ready to land
and it has a dependency on these.

thanks
-- PMM

Re: [PATCH 0/2] target/i386: Fix 0 * Inf + QNaN regression

[Paolo Bonzini]

On 2/3/25 12:05, Peter Maydell wrote:
> On Fri, 24 Jan 2025 at 17:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Queued, thanks.
> 
> Thanks; do you plan to send a pullreq with these in soon?
> I ask because the Arm FEAT_AFP set is now ready to land
> and it has a dependency on these.
I do but if you want to send it yourself, feel free to include them.

Paolo