Re: [Qemu-devel] [PATCH] fpu/softfloat: check for Inf / x or 0 / x before /0

[Emilio G. Cota]

On Tue, Apr 17, 2018 at 21:54:03 +0100, Peter Maydell wrote:
> On 17 April 2018 at 20:04, Emilio G. Cota <address@hidden> wrote:
> > Note that in fp-test I am not checking for flags that are raised
> > when none are expected, because doing so gives quite a few errors.
> > Just noticed that enabling this check yields 1049 of these errors for
> > v2.11, and before this patch that number was 1087. After this
> > patch, it is again brought down to 1049. IOW, the test cases in
> > fp-test raise exactly the same flags as v2.11, which is good to know.
> >
> > The 1049 errors are probably false positives -- at least a big
> > chunk of them should be, given that "-t host" gives even more errors.
> > I am tempted to keep the flag check and whitelist these errors
> > though, which would catch regressions such as the one we're fixing here.
> 
> I strongly suspect we do have a few cases where we get the answers
> wrong and/or don't report the flags right, so ideally we'd have
> a look at them in more detail...
> 
> > Here is the report file with the 1049 failing test cases:
> >   http://www.cs.columbia.edu/~cota/qemu/fp-test-after-inf-patch.txt
> 
> Syntax for interpreting the report:
> https://www.research.ibm.com/haifa/projects/verification/fpgen/syntax.txt
> 
> Here's the first one, am I reading it right?
> 
> + 0xffc00000 0xffb00000, expected: 0xffc00000, returned: 0xffc00000,
> expected exceptions: none, returned: i
> error: flags mismatch for input @ ibm/Basic-Types-Inputs.fptest:1346:
> b32+ =0 Q S -> Q
> 
> That's a float32 addition where the first input is a QNaN
> and the second an SNaN (presumably the test is configured
> for what in QEMU is snan_bit_is_one == 0), and it expects
> the result to be the QNaN, with no exceptions set. But
> we raise Invalid.
> =0 is the rounding mode, not relevant here.

Yes, you're reading it right.

> IEEE754-2008 s6.2 seems pretty clear that if there's
> an SNaN as an operand then operations like addition should
> signal Invalid. So this looks like a bug in the test case
> input. (Which is weird, because IBM must have tested this,
> so it's odd to see an obvious error in it.)

Yes sometimes the input files don't make much sense -- that's why
I ended up whitelisting some of them.

BTW I just checked with -t host on an IBM Power8, and we get
the same 1049 flag errors we get with -t soft plus two additional ones:

+A 0xffb00000, expected: 0x7fa00000, returned: 0x7fa00000, \
  expected exceptions: i, returned: none
+error: flags mismatch for input @ ibm/Basic-Types-Inputs.fptest:382:
+b32A =0 S -> S i
(...)
+cff 0xffb00000, expected: 0x7ff8000000000000, returned: 0x7ff4000000000000, \
  expected exceptions: i, returned: none
+error: flags mismatch for input @ ibm/Basic-Types-Inputs.fptest:26170:
+b32b64cff =0 S -> Q i

On x86 with -t host we again get a strict superset of what we get
with -t soft.

So yeah, I don't know what these test cases are about.

> Most of the "expected none, returned i" lines look
> like the same thing. We should look at the others, though.

Given the above, whitelisting the 1049 cases and forcing the flag
checks for all tests (as below) seems reasonable to me.

--- a/tests/fp/fp-test.c
+++ b/tests/fp/fp-test.c
@@ -247,7 +247,7 @@ static enum error tester_check(const struct test_op *t, 
uint64_t res64,
             goto out;
         }
     }
-    if (t->exceptions && flags != (t->exceptions | default_exceptions)) {
+    if (flags != (t->exceptions | default_exceptions)) {
         err = ERROR_EXCEPTIONS;
         goto out;
     }

Thanks,

                E.