[RFC PATCH] target/ppc: do not silence snan in xscvspdpn

* [RFC PATCH] target/ppc: do not silence snan in xscvspdpn
@ 2021-12-13 12:13 matheus.ferst
  2021-12-13 12:36 ` Philippe Mathieu-Daudé
  2021-12-13 15:46 ` Richard Henderson
  0 siblings, 2 replies; 8+ messages in thread
From: matheus.ferst @ 2021-12-13 12:13 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: peter.maydell, Matheus Ferst, danielhb413, groug, clg,
	alex.bennee, aurelien, david

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

The non-signalling versions of VSX scalar convert to shorter/longer
precision insns doesn't silence SNaNs in the hardware. We are currently
honoring this behavior in xscvdpspn, since helper_xscvdpspn handles the
conversion with extracts/deposits/etc. OTOH, xscvspdpn uses
float32_to_float64 that calls parts_float_to_float, which uses
parts_return_nan that finally calls parts_silence_nan if the input is an
SNaN.

To address this problem, this patch adds a new float_status flag
(return_snan) to avoid the call to parts_silence_nan in the
float_class_snan case of parts_return_nan.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
This behavior was observed in a Power9 and can be reproduced with the
following code:

int main(void)
{
    __uint128_t t, b = 0x7f80000200000000;

    asm("xscvspdpn %x0, %x1\n\t"
        : "=wa" (t)
        : "wa" (b << 64));
    printf("0x%016" PRIx64 "%016" PRIx64 "\n",
           (uint64_t)(t >> 64), (uint64_t)t);

    b = 0x7ff0000000000002;
    asm("xscvdpspn %x0, %x1\n\t"
        : "=wa" (t)
        : "wa" (b << 64));
    printf("0x%016" PRIx64 "%016" PRIx64 "\n",
           (uint64_t)(t >> 64), (uint64_t)t);

    return 0;
}

That results in:
$ ./xscv_non_signaling
xscvspdpn: 0x7ff00000400000000000000000000000
xscvdpspn: 0x7f8000007f8000000000000000000000
$ qemu-ppc64le ./xscv_non_signaling
xscvspdpn: 0x7ff80000400000000000000000000000
xscvdpspn: 0x7f8000007f8000000000000000000000

PowerISA is more descriptive of xscvdpspn wrt SNaN but doesn't say
anything about NaNs in xscvspdpn description. Should we match the
hardware behavior? If so, does it worth adding a new flag in
float_status for a single instruction? We could also handle that in
helper_xscvdpspn, e.g.:

float32 sp = xb >> 32;
float64 dp;

dp = float32_to_float64(xb >> 32, &tstat);
if (float32_classify(sp) == is_snan) {
    dp &= ~(1ULL << 51);
    dp |= (dp & 0x7ffffffffffffULL) == 0;
}

return dp;

but it feels kind hacky.
---
 fpu/softfloat-parts.c.inc      | 10 ++++++----
 fpu/softfloat-specialize.c.inc |  9 +++++++++
 include/fpu/softfloat-types.h  |  1 +
 target/ppc/fpu_helper.c        |  2 ++
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 41d4b17e41..ccc24f9153 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -20,10 +20,12 @@ static void partsN(return_nan)(FloatPartsN *a, float_status *s)
     switch (a->cls) {
     case float_class_snan:
         float_raise(float_flag_invalid, s);
-        if (s->default_nan_mode) {
-            parts_default_nan(a, s);
-        } else {
-            parts_silence_nan(a, s);
+        if (!return_snan(s)) {
+            if (s->default_nan_mode) {
+                parts_default_nan(a, s);
+            } else {
+                parts_silence_nan(a, s);
+            }
         }
         break;
     case float_class_qnan:
diff --git a/fpu/softfloat-specialize.c.inc b/fpu/softfloat-specialize.c.inc
index f2ad0f335e..70f686e0a3 100644
--- a/fpu/softfloat-specialize.c.inc
+++ b/fpu/softfloat-specialize.c.inc
@@ -92,6 +92,15 @@ static inline bool no_signaling_nans(float_status *status)
 #endif
 }
 
+static inline bool return_snan(float_status *status)
+{
+#if defined(TARGET_PPC)
+    return status->return_snan;
+#else
+    return false;
+#endif
+}
+
 /* Define how the architecture discriminates signaling NaNs.
  * This done with the most significant bit of the fraction.
  * In IEEE 754-1985 this was implementation defined, but in IEEE 754-2008
diff --git a/include/fpu/softfloat-types.h b/include/fpu/softfloat-types.h
index 5bcbd041f7..eb55298348 100644
--- a/include/fpu/softfloat-types.h
+++ b/include/fpu/softfloat-types.h
@@ -188,6 +188,7 @@ typedef struct float_status {
     bool snan_bit_is_one;
     bool use_first_nan;
     bool no_signaling_nans;
+    bool return_snan;
 } float_status;
 
 #endif /* SOFTFLOAT_TYPES_H */
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index c4896cecc8..9fc774fdb0 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2746,6 +2746,8 @@ uint64_t helper_xscvdpspn(CPUPPCState *env, uint64_t xb)
 uint64_t helper_xscvspdpn(CPUPPCState *env, uint64_t xb)
 {
     float_status tstat = env->fp_status;
+
+    tstat.return_snan = true;
     set_float_exception_flags(0, &tstat);
 
     return float32_to_float64(xb >> 32, &tstat);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread