diff mbox

tests/tcg: add test-i386-fprem and make target for comparing QEMU to h/w

Message ID 1342491857-22734-1-git-send-email-catalinp@google.com
State New
Headers show

Commit Message

Catalin Patulea July 17, 2012, 2:24 a.m. UTC 
I'm just sending this in as a draft for now. There are two outstanding issues:

1) What is the recommended type for bitfields? A quick grep shows inconsistent
usage of unsigned int/signed int/uintNN_t across the codebase. Note that
mantissa must be 63 bits so int won't do on either 32 or 64-bit builds.

2) Intel says that FPREM can sometimes generate numeric underflow (#U). I've
been trying to come up with a concrete case, asked around, etc, for a few days
now and I haven't come up with anything. It comes down to, I don't think FPREM
can ever lose precision with respect to its operands. Can you come up with a
numeric underflow case?

--- draft commit message: ---
This test contains manual and automatically-generated test cases, in the style
of test-i386 (run on h/w, run on QEMU, diff the outputs), for the FPREM
instruction. For each test case, the input operands, result and FPU status word
are printed.

The manual cases are inspired from the instruction spec and get basic coverage
of all exceptions FPREM can generate. They also test some "special" exceptions
like stack underflow (when the FPU stack contains only one operand).

The automated cases are generated from all pairs of extreme values in the
bitfields that make up the input operands. This gives rise to a range of
numeric values, NaNs, infinities, and other special values. FPREM raises
either no exceptions, invalid or denormal, and these cases are intended to get
good code path coverage of the implementation.

Signed-off-by: Catalin Patulea <catalinp@google.com>
---
 tests/tcg/Makefile          |    9 ++
 tests/tcg/test-i386-fprem.c |  325 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 334 insertions(+), 0 deletions(-)
 create mode 100644 tests/tcg/test-i386-fprem.c

Comments

Catalin Patulea July 17, 2012, 2:33 a.m. UTC | #1 
On Mon, Jul 16, 2012 at 10:24 PM, Catalin Patulea <catalinp@google.com> wrote:
> I don't think FPREM
> can ever lose precision with respect to its operands.
Sorry, this should be, "FPREM can't _gain_ precision" with respect to
its operands.

Think of the binary representation with infinite precision, where the
exponent determines the position of the binary point, the binary
points are aligned between the two inputs, and the stored mantissa is
padded to the right with 0s. I don't see how FPREM could inject bits
to the right of the least significant LSB between the two inputs.
Peter Maydell July 17, 2012, 12:23 p.m. UTC | #2 
On 17 July 2012 03:24, Catalin Patulea <catalinp@google.com> wrote:
> I'm just sending this in as a draft for now. There are two outstanding issues:
>
> 1) What is the recommended type for bitfields? A quick grep shows inconsistent
> usage of unsigned int/signed int/uintNN_t across the codebase. Note that
> mantissa must be 63 bits so int won't do on either 32 or 64-bit builds.

I think our current position is "don't use bitfields for anything that
has to match an externally defined layout". This is because on Windows
we have to compile with -mms-bitfields [for compatibility with Windows
APIs], which can lead to different layouts for structs with bitfields.
For instance in your struct:

> +    struct {
> +        unsigned long long mantissa:63;
> +        unsigned int one:1;
> +        unsigned int exponent:15;
> +        unsigned int negative:1;
> +        unsigned int empty:16;
> +    } ieee;

the Windows build would not put the fields 'mantissa' and
'one' in the same 64 bit word, it would insert a padding bit.

-- PMM
Catalin Patulea July 17, 2012, 2:49 p.m. UTC | #3 
On Tue, Jul 17, 2012 at 8:23 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> I think our current position is "don't use bitfields for anything that
> has to match an externally defined layout". This is because on Windows
> we have to compile with -mms-bitfields [for compatibility with Windows
> APIs], which can lead to different layouts for structs with bitfields.
> For instance in your struct:
>
>> +    struct {
>> +        unsigned long long mantissa:63;
>> +        unsigned int one:1;
>> +        unsigned int exponent:15;
>> +        unsigned int negative:1;
>> +        unsigned int empty:16;
>> +    } ieee;
>
> the Windows build would not put the fields 'mantissa' and
> 'one' in the same 64 bit word, it would insert a padding bit.
Since floatx80 is not part of a Windows API (in fact, it's not part of
any API, it's in a test), could I give the structs an attribute of
"gcc_struct"?

http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html (under
"6.36.5 i386 Variable Attributes")
Catalin Patulea July 20, 2012, 10:48 p.m. UTC | #4 
On Tue, Jul 17, 2012 at 10:49 AM, Catalin Patulea <catalinp@google.com> wrote:
> > the Windows build would not put the fields 'mantissa' and
> > 'one' in the same 64 bit word, it would insert a padding bit.
> Since floatx80 is not part of a Windows API (in fact, it's not part of
> any API, it's in a test), could I give the structs an attribute of
> "gcc_struct"?
I just noticed that QEMU_PACKED includes this attribute whenever it is
needed (#if defined(_WIN32)) since Stefan Weil's
0f7fdd347514ea97b24f5f658f3ae31f9b078397. Would making the float80u
structs QEMU_PACKED be a reasonable solution? Stefan, can you comment
on whether this is an appropriate use of QEMU_PACKED?

Are there any other issues with the patch? I would punt on the numeric
underflow issue (leaving a TODO), because I think even without it,
adding this test to the codebase is a significant improvement over
what's there currently.

Also, ultimately, this is just a safety net for my patch which changes
the FPREM implementation, so I would want to use the testing patch to
make progress there.
Peter Maydell July 21, 2012, 9:03 a.m. UTC | #5 
On 20 July 2012 23:48, Catalin Patulea <catalinp@google.com> wrote:
> On Tue, Jul 17, 2012 at 10:49 AM, Catalin Patulea <catalinp@google.com> wrote:
>> > the Windows build would not put the fields 'mantissa' and
>> > 'one' in the same 64 bit word, it would insert a padding bit.
>> Since floatx80 is not part of a Windows API (in fact, it's not part of
>> any API, it's in a test), could I give the structs an attribute of
>> "gcc_struct"?
> I just noticed that QEMU_PACKED includes this attribute whenever it is
> needed (#if defined(_WIN32)) since Stefan Weil's
> 0f7fdd347514ea97b24f5f658f3ae31f9b078397. Would making the float80u
> structs QEMU_PACKED be a reasonable solution? Stefan, can you comment
> on whether this is an appropriate use of QEMU_PACKED?

I think the general rule would be "don't use bitfields at all where
you care about layout in memory". Since this is only test case code
we might be more relaxed about it, though.

I don't have any other issues with the patch and I'm happy to leave
TODOs about missing areas of testing; as you say, some testing is
better than none.

-- PMM
diff mbox

Patch

diff --git a/tests/tcg/Makefile b/tests/tcg/Makefile
index 9ff47b8..06cd563 100644
--- a/tests/tcg/Makefile
+++ b/tests/tcg/Makefile
@@ -22,6 +22,7 @@  I386_TESTS=hello-i386 \
 	   testthread \
 	   sha1-i386 \
 	   test-i386 \
+	   test-i386-fprem \
 	   test-mmap \
 	   # runcom
 
@@ -55,6 +56,11 @@  run-test-i386: test-i386
 	-$(QEMU) test-i386 > test-i386.out
 	@if diff -u test-i386.ref test-i386.out ; then echo "Auto Test OK"; fi
 
+run-test-i386-fprem: test-i386-fprem
+	./test-i386-fprem > test-i386-fprem.ref
+	-$(QEMU) test-i386-fprem > test-i386-fprem.out
+	@if diff -u test-i386-fprem.ref test-i386-fprem.out ; then echo "Auto Test OK"; fi
+
 run-test-x86_64: test-x86_64
 	./test-x86_64 > test-x86_64.ref
 	-$(QEMU_X86_64) test-x86_64 > test-x86_64.out
@@ -93,6 +99,9 @@  test-i386: test-i386.c test-i386-code16.S test-i386-vm86.S \
 	$(CC_I386) $(QEMU_INCLUDES) $(CFLAGS) $(LDFLAGS) -o $@ \
               $(<D)/test-i386.c $(<D)/test-i386-code16.S $(<D)/test-i386-vm86.S -lm
 
+test-i386-fprem: test-i386-fprem.c
+	$(CC_I386) $(QEMU_INCLUDES) $(CFLAGS) $(LDFLAGS) -o $@ $^
+
 test-x86_64: test-i386.c \
            test-i386.h test-i386-shift.h test-i386-muldiv.h
 	$(CC_X86_64) $(CFLAGS) $(LDFLAGS) -o $@ $(<D)/test-i386.c -lm
diff --git a/tests/tcg/test-i386-fprem.c b/tests/tcg/test-i386-fprem.c
new file mode 100644
index 0000000..ed3cbaa
--- /dev/null
+++ b/tests/tcg/test-i386-fprem.c
@@ -0,0 +1,325 @@ 
+/*
+ *  x86 FPREM test - executes the FPREM instruction with various corner case
+ *  operands and prints the operands, result and FPU status word.
+ *
+ *  Run this on real hardware, and under QEMU, to verify QEMU correctness.
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *  Copyright (c) 2012 Catalin Patulea
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "compiler.h"
+#include "osdep.h"
+#include <stdio.h>
+#include <inttypes.h>
+
+/*
+ * Inspired by <ieee754.h>'s union ieee854_long_double, but with single
+ * long long mantissa fields and assuming little-endianness for simplicity.
+ */
+union float80u {
+    long double d;
+
+    /* This is the IEEE 854 double-extended-precision format.  */
+    struct {
+        unsigned long long mantissa:63;
+        unsigned int one:1;
+        unsigned int exponent:15;
+        unsigned int negative:1;
+        unsigned int empty:16;
+    } ieee;
+
+    /* This is for NaNs in the IEEE 854 double-extended-precision format.  */
+    struct {
+        unsigned long long mantissa:62;
+        unsigned int quiet_nan:1;
+        unsigned int one:1;
+        unsigned int exponent:15;
+        unsigned int negative:1;
+        unsigned int empty:16;
+    } ieee_nan;
+};
+
+#define IEEE854_LONG_DOUBLE_BIAS 0x3fff
+
+static const union float80u q_nan = {
+    .ieee_nan.negative = 0,  /* X */
+    .ieee_nan.exponent = 0x7fff,
+    .ieee_nan.one = 1,
+    .ieee_nan.quiet_nan = 1,
+    .ieee_nan.mantissa = 0,
+};
+
+static const union float80u s_nan = {
+    .ieee_nan.negative = 0,  /* X */
+    .ieee_nan.exponent = 0x7fff,
+    .ieee_nan.one = 1,
+    .ieee_nan.quiet_nan = 0,
+    .ieee_nan.mantissa = 1,  /* nonzero */
+};
+
+static const union float80u pos_inf = {
+    .ieee.negative = 0,
+    .ieee.exponent = 0x7fff,
+    .ieee.one = 1,
+    .ieee.mantissa = 0,
+};
+
+static const union float80u pseudo_pos_inf = {  /* "unsupported" */
+    .ieee.negative = 0,
+    .ieee.exponent = 0x7fff,
+    .ieee.one = 0,
+    .ieee.mantissa = 0,
+};
+
+static const union float80u pos_denorm = {
+    .ieee.negative = 0,
+    .ieee.exponent = 0,
+    .ieee.one = 0,
+    .ieee.mantissa = 1,
+};
+
+static const union float80u smallest_positive_norm = {
+    .ieee.negative = 0,
+    .ieee.exponent = 1,
+    .ieee.one = 1,
+    .ieee.mantissa = 0,
+};
+
+static void fninit()
+{
+    asm volatile ("fninit\n");
+}
+
+static long double fprem(long double a, long double b, uint16_t *sw)
+{
+    long double result;
+    asm volatile ("fprem\n"
+                  "fnstsw %1\n"
+                  : "=t" (result), "=m" (*sw)
+                  : "0" (a), "u" (b)
+                  : "st(1)");
+    return result;
+}
+
+#define FPUS_IE (1 << 0)
+#define FPUS_DE (1 << 1)
+#define FPUS_ZE (1 << 2)
+#define FPUS_OE (1 << 3)
+#define FPUS_UE (1 << 4)
+#define FPUS_PE (1 << 5)
+#define FPUS_SF (1 << 6)
+#define FPUS_SE (1 << 7)
+#define FPUS_C0 (1 << 8)
+#define FPUS_C1 (1 << 9)
+#define FPUS_C2 (1 << 10)
+#define FPUS_TOP 0x3800
+#define FPUS_C3 (1 << 14)
+#define FPUS_B  (1 << 15)
+
+#define FPUS_EMASK 0x007f
+
+#define FPUC_EM 0x3f
+
+static void psw(uint16_t sw)
+{
+    printf("SW:  C3 TopC2C1C0\n");
+    printf("SW: %c %d %3d %d %d %d %c %c %c %c %c %c %c %c\n",
+           sw & FPUS_B ? 'B' : 'b',
+           !!(sw & FPUS_C3),
+           (sw & FPUS_TOP) >> 11,
+           !!(sw & FPUS_C2),
+           !!(sw & FPUS_C1),
+           !!(sw & FPUS_C0),
+           (sw & FPUS_SE) ? 'S' : 's',
+           (sw & FPUS_SF) ? 'F' : 'f',
+           (sw & FPUS_PE) ? 'P' : 'p',
+           (sw & FPUS_UE) ? 'U' : 'u',
+           (sw & FPUS_OE) ? 'O' : 'o',
+           (sw & FPUS_ZE) ? 'Z' : 'z',
+           (sw & FPUS_DE) ? 'D' : 'd',
+           (sw & FPUS_IE) ? 'I' : 'i');
+}
+
+static void do_fprem(long double a, long double b)
+{
+    const union float80u au = {.d = a};
+    const union float80u bu = {.d = b};
+    union float80u ru;
+    uint16_t sw;
+
+    printf("A: S=%d Exp=%04x Int=%d (QNaN=%d) Sig=%016llx (%.06Le)\n",
+           au.ieee.negative, au.ieee.exponent, au.ieee.one,
+           au.ieee_nan.quiet_nan, (unsigned long long)au.ieee.mantissa,
+           a);
+    printf("B: S=%d Exp=%04x Int=%d (QNaN=%d) Sig=%016llx (%.06Le)\n",
+           bu.ieee.negative, bu.ieee.exponent, bu.ieee.one,
+           bu.ieee_nan.quiet_nan, (unsigned long long)bu.ieee.mantissa,
+           b);
+
+    fninit();
+    ru.d = fprem(a, b, &sw);
+    psw(sw);
+
+    printf("R: S=%d Exp=%04x Int=%d (QNaN=%d) Sig=%016llx (%.06Le)\n",
+           ru.ieee.negative, ru.ieee.exponent, ru.ieee.one,
+           ru.ieee_nan.quiet_nan, (unsigned long long)ru.ieee.mantissa,
+           ru.d);
+    printf("\n");
+}
+
+static void do_fprem_stack_underflow(void)
+{
+    const long double a = 1.0;
+    union float80u ru;
+    uint16_t sw;
+
+    fninit();
+    asm volatile ("fprem\n"
+                  "fnstsw %1\n"
+                  : "=t" (ru.d), "=m" (sw)
+                  : "0" (a)
+                  : "st(1)");
+    psw(sw);
+
+    printf("R: S=%d Exp=%04x Int=%d (QNaN=%d) Sig=%016llx (%.06Le)\n",
+           ru.ieee.negative, ru.ieee.exponent, ru.ieee.one,
+           ru.ieee_nan.quiet_nan, (unsigned long long)ru.ieee.mantissa,
+           ru.d);
+    printf("\n");
+}
+
+static void test_fprem_cases(void)
+{
+    printf("= stack underflow =\n");
+    do_fprem_stack_underflow();
+
+    printf("= invalid operation =\n");
+    do_fprem(s_nan.d, 1.0);
+    do_fprem(1.0, 0.0);
+    do_fprem(pos_inf.d, 1.0);
+    do_fprem(pseudo_pos_inf.d, 1.0);
+
+    printf("= denormal =\n");
+    do_fprem(pos_denorm.d, 1.0);
+    do_fprem(1.0, pos_denorm.d);
+
+    /* printf("= underflow =\n"); */
+    /* TODO(catalinp): Is there a case where FPREM raises underflow? */
+}
+
+static void test_fprem_pairs(void)
+{
+    unsigned int negative_index_a = 0;
+    unsigned int negative_index_b = 0;
+    static const unsigned int negative_values[] = {
+        0,
+        1,
+    };
+
+    unsigned int exponent_index_a = 0;
+    unsigned int exponent_index_b = 0;
+    static const unsigned int exponent_values[] = {
+        0,
+        1,
+        2,
+        IEEE854_LONG_DOUBLE_BIAS - 1,
+        IEEE854_LONG_DOUBLE_BIAS,
+        IEEE854_LONG_DOUBLE_BIAS + 1,
+        0x7ffd,
+        0x7ffe,
+        0x7fff,
+    };
+
+    unsigned int one_index_a = 0;
+    unsigned int one_index_b = 0;
+    static const unsigned int one_values[] = {
+        0,
+        1,
+    };
+
+    unsigned int quiet_nan_index_a = 0;
+    unsigned int quiet_nan_index_b = 0;
+    static const unsigned int quiet_nan_values[] = {
+        0,
+        1,
+    };
+
+    unsigned int mantissa_index_a = 0;
+    unsigned int mantissa_index_b = 0;
+    static const unsigned long long mantissa_values[] = {
+        0,
+        1,
+        2,
+        0x3ffffffffffffffdULL,
+        0x3ffffffffffffffeULL,
+        0x3fffffffffffffffULL,
+    };
+
+    for (;;) {
+#define INIT_FIELD(var, field) \
+            .ieee_nan.field = field##_values[field##_index_##var]
+        const union float80u a = {
+            INIT_FIELD(a, negative),
+            INIT_FIELD(a, exponent),
+            INIT_FIELD(a, one),
+            INIT_FIELD(a, quiet_nan),
+            INIT_FIELD(a, mantissa),
+        };
+        const union float80u b = {
+            INIT_FIELD(b, negative),
+            INIT_FIELD(b, exponent),
+            INIT_FIELD(b, one),
+            INIT_FIELD(b, quiet_nan),
+            INIT_FIELD(b, mantissa),
+        };
+#undef INIT_FIELD
+
+        do_fprem(a.d, b.d);
+
+        int carry = 1;
+#define CARRY_INTO(var, field) do { \
+            if (carry) { \
+                if (++field##_index_##var == ARRAY_SIZE(field##_values)) { \
+                    field##_index_##var = 0; \
+                } else { \
+                    carry = 0; \
+                } \
+            } \
+        } while (0)
+        CARRY_INTO(b, mantissa);
+        CARRY_INTO(b, quiet_nan);
+        CARRY_INTO(b, one);
+        CARRY_INTO(b, exponent);
+        CARRY_INTO(b, negative);
+        CARRY_INTO(a, mantissa);
+        CARRY_INTO(a, quiet_nan);
+        CARRY_INTO(a, one);
+        CARRY_INTO(a, exponent);
+        CARRY_INTO(a, negative);
+#undef CARRY_INTO
+
+        if (carry) {
+            break;
+        }
+    }
+}
+
+int main(int argc, char **argv)
+{
+    test_fprem_cases();
+    test_fprem_pairs();
+    return 0;
+}