Sending this respin (little more than a rebase) in case there's
reviewer bandwidth available until the soft-freeze in 3 weeks.

v3: https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg00455.html

Not much has changed since April's v3:

- Rebase to current master, fixing some conflicts.
- Add R-b tags
- Fix a typo in the long "hardfloat" comment.
- Move <system.h> header includes right after #include "qemu/osdep.h"
  Did this after seeing this message from Eric Blake:
  https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg00831.html

Note: there are some checkpatch errors, but they are false positives.

Perf numbers for fp-bench are in each commit log; numbers for several
benchmarks are in the last patch's commit log.

You can fetch this series from:
  https://github.com/cota/qemu/tree/hardfloat-v4

Thanks,

		Emilio

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1528768140-17894-1-git-send-email-cota@braap.org
Subject: [Qemu-devel] [PATCH v4 00/14] fp-test + hardfloat

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1528768140-17894-1-git-send-email-cota@braap.org -> patchew/1528768140-17894-1-git-send-email-cota@braap.org
Switched to a new branch 'test'
a08c574e65 hardfloat: support float32/64 comparison
65f23ce4e3 hardfloat: support float32/64 square root
5f21e531b7 hardfloat: support float32/64 fused multiply-add
9338404097 hardfloat: support float32/64 division
2d8fc0ba02 hardfloat: support float32/64 multiplication
896efd9cf9 hardfloat: support float32/64 addition and subtraction
79c0be0919 fpu: introduce hardfloat
c5c6cb188b softfloat: add float{32, 64}_is_zero_or_normal
e30f225172 softfloat: rename canonicalize to sf_canonicalize
b37db7a121 tests/fp: add fp-bench, a collection of simple floating point microbenchmarks
f19bf44348 target/tricore: use float32_is_denormal
ebf05b0fee softfloat: add float{32, 64}_is_{de, }normal
64bf1ba6a7 fp-test: add muladd variants
2ca163d26a tests: add fp-test, a floating point test suite

=== OUTPUT BEGIN ===
Checking PATCH 1/14: tests: add fp-test, a floating point test suite...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#107: 
new file mode 100644

ERROR: Macros with complex values should be enclosed in parenthesis
#379: FILE: tests/fp/fp-test.c:219:
+#define PR_EXCEPTIONS(x)                                \
+        ((x) & STANDARD_EXCEPTIONS ? "" : "none"),      \
+        (((x) & float_flag_inexact)   ? "x" : ""),      \
+        (((x) & float_flag_underflow) ? "u" : ""),      \
+        (((x) & float_flag_overflow)  ? "o" : ""),      \
+        (((x) & float_flag_divbyzero) ? "z" : ""),      \
+        (((x) & float_flag_invalid)   ? "i" : "")

ERROR: consider using qemu_strtoul in preference to strtoul
#840: FILE: tests/fp/fp-test.c:680:
+            significand = strtoul(&p[3], &pos, 16);

ERROR: consider using qemu_strtol in preference to strtol
#845: FILE: tests/fp/fp-test.c:685:
+            exponent = strtol(pos, &pos, 10) + 127;

ERROR: consider using qemu_strtoul in preference to strtoul
#870: FILE: tests/fp/fp-test.c:710:
+            significand = strtoul(&p[3], &pos, 16);

ERROR: consider using qemu_strtol in preference to strtol
#875: FILE: tests/fp/fp-test.c:715:
+            exponent = strtol(pos, &pos, 10) + 1023;

ERROR: consider using qemu_strtof in preference to strtof
#894: FILE: tests/fp/fp-test.c:734:
+            float f = strtof(p, &pos);

total: 6 errors, 1 warnings, 1218 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/14: fp-test: add muladd variants...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#69: 
new file mode 100644

total: 0 errors, 1 warnings, 99 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/14: softfloat: add float{32, 64}_is_{de, }normal...
Checking PATCH 4/14: target/tricore: use float32_is_denormal...
Checking PATCH 5/14: tests/fp: add fp-bench, a collection of simple floating point microbenchmarks...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#46: 
new file mode 100644

total: 0 errors, 1 warnings, 545 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/14: softfloat: rename canonicalize to sf_canonicalize...
Checking PATCH 7/14: softfloat: add float{32, 64}_is_zero_or_normal...
Checking PATCH 8/14: fpu: introduce hardfloat...
ERROR: spaces required around that '*' (ctx:WxV)
#95: FILE: fpu/softfloat.c:143:
+    static inline void name(soft_t *a, float_status *s)                 \
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#109: FILE: fpu/softfloat.c:157:
+    static inline void name(soft_t *a, float_status *s) \
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#122: FILE: fpu/softfloat.c:170:
+    static inline void name(soft_t *a, soft_t *b, float_status *s)      \
                                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#136: FILE: fpu/softfloat.c:184:
+    static inline void name(soft_t *a, soft_t *b, soft_t *c, float_status *s) \
                                                                           ^

ERROR: spaces required around that '*' (ctx:WxV)
#150: FILE: fpu/softfloat.c:198:
+static inline bool can_use_fpu(const float_status *s)
                                                   ^

WARNING: architecture specific defines should be avoided
#161: FILE: fpu/softfloat.c:209:
+#if defined(__x86_64__)

WARNING: architecture specific defines should be avoided
#183: FILE: fpu/softfloat.c:231:
+#if defined(__x86_64__) || defined(__aarch64__)

total: 5 errors, 2 warnings, 353 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/14: hardfloat: support float32/64 addition and subtraction...
ERROR: spaces required around that '*' (ctx:WxV)
#74: FILE: fpu/softfloat.c:1081:
+soft_float32_add(float32 a, float32 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#85: FILE: fpu/softfloat.c:1091:
+soft_float64_add(float64 a, float64 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#96: FILE: fpu/softfloat.c:1111:
+soft_float32_sub(float32 a, float32 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#107: FILE: fpu/softfloat.c:1121:
+soft_float64_sub(float64 a, float64 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#157: FILE: fpu/softfloat.c:1172:
+static float32 float32_addsub(float32 a, float32 b, float_status *s,
                                                                  ^

ERROR: spaces required around that '*' (ctx:WxV)
#169: FILE: fpu/softfloat.c:1184:
+static float64 float64_addsub(float64 a, float64 b, float_status *s,
                                                                  ^

ERROR: spaces required around that '*' (ctx:WxV)
#182: FILE: fpu/softfloat.c:1197:
+float32_add(float32 a, float32 b, float_status *s)
                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#188: FILE: fpu/softfloat.c:1203:
+float32_sub(float32 a, float32 b, float_status *s)
                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#194: FILE: fpu/softfloat.c:1209:
+float64_add(float64 a, float64 b, float_status *s)
                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#200: FILE: fpu/softfloat.c:1215:
+float64_sub(float64 a, float64 b, float_status *s)
                                                ^

total: 10 errors, 0 warnings, 136 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 10/14: hardfloat: support float32/64 multiplication...
ERROR: spaces required around that '*' (ctx:WxV)
#46: FILE: fpu/softfloat.c:1280:
+soft_float32_mul(float32 a, float32 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#57: FILE: fpu/softfloat.c:1290:
+soft_float64_mul(float64 a, float64 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#85: FILE: fpu/softfloat.c:1319:
+static float32 f32_mul_fast_op(float32 a, float32 b, float_status *s)
                                                                   ^

ERROR: spaces required around that '*' (ctx:WxV)
#92: FILE: fpu/softfloat.c:1326:
+static float64 f64_mul_fast_op(float64 a, float64 b, float_status *s)
                                                                   ^

ERROR: spaces required around that '*' (ctx:WxV)
#100: FILE: fpu/softfloat.c:1334:
+float32_mul(float32 a, float32 b, float_status *s)
                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#112: FILE: fpu/softfloat.c:1346:
+float64_mul(float64 a, float64 b, float_status *s)
                                                ^

total: 6 errors, 0 warnings, 84 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/14: hardfloat: support float32/64 division...
ERROR: spaces required around that '*' (ctx:WxV)
#48: FILE: fpu/softfloat.c:1663:
+soft_float32_div(float32 a, float32 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#58: FILE: fpu/softfloat.c:1673:
+soft_float64_div(float64 a, float64 b, float_status *status)
                                                     ^

ERROR: spaces required around that '*' (ctx:WxV)
#125: FILE: fpu/softfloat.c:1741:
+float32_div(float32 a, float32 b, float_status *s)
                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#137: FILE: fpu/softfloat.c:1753:
+float64_div(float64 a, float64 b, float_status *s)
                                                ^

total: 4 errors, 0 warnings, 106 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 12/14: hardfloat: support float32/64 fused multiply-add...
ERROR: spaces required around that '*' (ctx:WxV)
#50: FILE: fpu/softfloat.c:1573:
+                    float_status *status)
                                  ^

ERROR: spaces required around that '*' (ctx:WxV)
#62: FILE: fpu/softfloat.c:1585:
+                    float_status *status)
                                  ^

ERROR: spaces required around that '*' (ctx:WxV)
#77: FILE: fpu/softfloat.c:1603:
+    name(soft_t a, soft_t b, soft_t c, int flags, float_status *s)      \
                                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#137: FILE: fpu/softfloat.c:1663:
+    name(soft_t a, soft_t b, soft_t c, int flags, float_status *s)      \
                                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#209: FILE: fpu/softfloat.c:1735:
+float32_muladd(float32 a, float32 b, float32 c, int flags, float_status *s)
                                                                         ^

ERROR: spaces required around that '*' (ctx:WxV)
#219: FILE: fpu/softfloat.c:1745:
+float64_muladd(float64 a, float64 b, float64 c, int flags, float_status *s)
                                                                         ^

total: 6 errors, 0 warnings, 187 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 13/14: hardfloat: support float32/64 square root...
ERROR: spaces required around that '*' (ctx:WxV)
#49: FILE: fpu/softfloat.c:2816:
+soft_float32_sqrt(float32 a, float_status *status)
                                           ^

ERROR: spaces required around that '*' (ctx:WxV)
#58: FILE: fpu/softfloat.c:2824:
+soft_float64_sqrt(float64 a, float_status *status)
                                           ^

ERROR: spaces required around that '*' (ctx:WxV)
#67: FILE: fpu/softfloat.c:2905:
+    static soft_t name(soft_t a, float_status *s)                       \
                                               ^

ERROR: spaces required around that '*' (ctx:WxV)
#86: FILE: fpu/softfloat.c:2924:
+    static soft_t name(soft_t a, float_status *s)                       \
                                               ^

ERROR: spaces required around that '*' (ctx:WxV)
#115: FILE: fpu/softfloat.c:2953:
+float32 __attribute__((flatten)) float32_sqrt(float32 a, float_status *s)
                                                                       ^

ERROR: spaces required around that '*' (ctx:WxV)
#124: FILE: fpu/softfloat.c:2962:
+float64 __attribute__((flatten)) float64_sqrt(float64 a, float_status *s)
                                                                       ^

total: 6 errors, 0 warnings, 91 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 14/14: hardfloat: support float32/64 comparison...
ERROR: spaces required around that '*' (ctx:WxV)
#113: FILE: fpu/softfloat.c:2676:
+name(float ## sz a, float ## sz b, bool is_quiet, float_status *s)      \
                                                                ^

ERROR: spaces required around that '*' (ctx:WxV)
#138: FILE: fpu/softfloat.c:2690:
+float16_compare(float16 a, float16 b, float_status *s)
                                                    ^

total: 2 errors, 0 warnings, 88 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

On Mon, Jun 11, 2018 at 21:48:46 -0400, Emilio G. Cota wrote:
> Sending this respin (little more than a rebase) in case there's
> reviewer bandwidth available until the soft-freeze in 3 weeks.

*ping*

Would be great to get this in for 3.1.

BTW I have a paper under submission that covers this work among
other improvements to QEMU. Reading this paper might ease a
subsequent review of this patch series; ask me privately if
you want to get a copy.

Thanks,

		Emilio

Emilio G. Cota <cota@braap.org> writes:

> On Mon, Jun 11, 2018 at 21:48:46 -0400, Emilio G. Cota wrote:
>> Sending this respin (little more than a rebase) in case there's
>> reviewer bandwidth available until the soft-freeze in 3 weeks.
>
> *ping*
>
> Would be great to get this in for 3.1.

I would like this merged by 3.1 as well. However I think there is still
some work to be done on the testing side. IIRC the fptest case works
with whitelists and I'd like to understand more about why we can't use
the whole test corpus? Is it testing features we don't have on all
architectures or just because it wouldn't pass because of holes in our
current softfloat?

Our experience of SVE has shown that despite the fairly extensive
testing we did there are still a bunch of corner cases we missed.
Hopefully the last few patches have fixed that but I guess it pays to be
exhaustive.

We now have the check-tcg infrastructure in place so it would be nice to
have proper native tests in place for each architecture. My experience
of the fcvt.c test case however is you end up using inline assembler to
ensure you exercise the right guest opcodes which makes it hard to
generalise for lots of architectures. I had written a bunch of patches
against the fptest to get it built under check-tcg but it was painful:

  * a bit hacky to build as unit test and as tcg test
  * needed a lot of boilerplate for each new operation

> BTW I have a paper under submission that covers this work among
> other improvements to QEMU. Reading this paper might ease a
> subsequent review of this patch series; ask me privately if
> you want to get a copy.
>
> Thanks,
>
> 		Emilio


--
Alex Bennée

On Tue, Aug 14, 2018 at 11:17:03 +0100, Alex Bennée wrote:
> Emilio G. Cota <cota@braap.org> writes:
> > Would be great to get this in for 3.1.
> 
> I would like this merged by 3.1 as well. However I think there is still
> some work to be done on the testing side. IIRC the fptest case works
> with whitelists and I'd like to understand more about why we can't use
> the whole test corpus? Is it testing features we don't have on all
> architectures or just because it wouldn't pass because of holes in our
> current softfloat?

Some test patterns are just strange. For instance:

d64+ =0 -1e-398 +0e-398 -> -1e-398

I think the IBM implementation uses 128 bits and then truncates to
whatever precision is required (64b in this case), so those tests
might make sense then. But for us, those tests don't make any sense.

The use of whitelists is a temporary workaround to avoid those weird
test patterns. The right fix is to keep our own set of test patterns,
without needing whitelisting.
BTW with this patchset we use 76572 out of 130471 test patterns, which
isn't bad at all. The whitelist is currently only 2% of the 130K.

> Our experience of SVE has shown that despite the fairly extensive
> testing we did there are still a bunch of corner cases we missed.
> Hopefully the last few patches have fixed that but I guess it pays to be
> exhaustive.

Agreed. That's why I wrote fp-test (and BTW found a bug in softfloat
thanks to it.)

> We now have the check-tcg infrastructure in place so it would be nice to
> have proper native tests in place for each architecture. My experience
> of the fcvt.c test case however is you end up using inline assembler to
> ensure you exercise the right guest opcodes which makes it hard to
> generalise for lots of architectures.

I think testing using assembly is necessary, but not sufficient.
That's why having tests that test the FP primitives directly
(like fp-test does with `-t soft`) is valuable, since you can
trivially exercise corner cases. Then you have to test that the
ISA's decoder does the right thing, but that's a separate test.

> I had written a bunch of patches
> against the fptest to get it built under check-tcg but it was painful:
> 
>   * needed a lot of boilerplate for each new operation

That depends on the op. If you want to test anything other than 32/64b
ops, then yes, you need to add some boilerplate. But otherwise it
is quite simple, for instance see patch 2.

>   * a bit hacky to build as unit test and as tcg test

It's not clear to me what the value as a TCG test is; each ISA
would have its own set of test patterns (and this set is distinct
from the test patterns we're using here, since those are only
a subset of the 754 standard).

So, my proposal for a v5:

- Commit the test files we need, instead of downloading them from
  the web. No whitelisting/exceptions except for tininess
  detection, which is necessary.

- fp-test is added to make test. This is a unit test of softfloat,
  not a TCG unit test.

- We defer TCG unit tests of FP to a later time.

How does that sound?

		Emilio

Emilio G. Cota <cota@braap.org> writes:

> On Tue, Aug 14, 2018 at 11:17:03 +0100, Alex Bennée wrote:
>> Emilio G. Cota <cota@braap.org> writes:
>> > Would be great to get this in for 3.1.
>>
>> I would like this merged by 3.1 as well. However I think there is still
>> some work to be done on the testing side. IIRC the fptest case works
>> with whitelists and I'd like to understand more about why we can't use
>> the whole test corpus? Is it testing features we don't have on all
>> architectures or just because it wouldn't pass because of holes in our
>> current softfloat?
>
> Some test patterns are just strange. For instance:
>
> d64+ =0 -1e-398 +0e-398 -> -1e-398
>
> I think the IBM implementation uses 128 bits and then truncates to
> whatever precision is required (64b in this case), so those tests
> might make sense then. But for us, those tests don't make any sense.
>
> The use of whitelists is a temporary workaround to avoid those weird
> test patterns. The right fix is to keep our own set of test patterns,
> without needing whitelisting.
> BTW with this patchset we use 76572 out of 130471 test patterns, which
> isn't bad at all. The whitelist is currently only 2% of the 130K.
>
>> Our experience of SVE has shown that despite the fairly extensive
>> testing we did there are still a bunch of corner cases we missed.
>> Hopefully the last few patches have fixed that but I guess it pays to be
>> exhaustive.
>
> Agreed. That's why I wrote fp-test (and BTW found a bug in softfloat
> thanks to it.)
>
>> We now have the check-tcg infrastructure in place so it would be nice to
>> have proper native tests in place for each architecture. My experience
>> of the fcvt.c test case however is you end up using inline assembler to
>> ensure you exercise the right guest opcodes which makes it hard to
>> generalise for lots of architectures.
>
> I think testing using assembly is necessary, but not sufficient.
> That's why having tests that test the FP primitives directly
> (like fp-test does with `-t soft`) is valuable, since you can
> trivially exercise corner cases. Then you have to test that the
> ISA's decoder does the right thing, but that's a separate test.
>
>> I had written a bunch of patches
>> against the fptest to get it built under check-tcg but it was painful:
>>
>>   * needed a lot of boilerplate for each new operation
>
> That depends on the op. If you want to test anything other than 32/64b
> ops, then yes, you need to add some boilerplate. But otherwise it
> is quite simple, for instance see patch 2.

Well half-precision is the next obvious thing that needs adding. If we
ever re-factor the rest of the code for our weird 80 bit float cousins
that will need adding as well.

>
>>   * a bit hacky to build as unit test and as tcg test
>
> It's not clear to me what the value as a TCG test is; each ISA
> would have its own set of test patterns (and this set is distinct
> from the test patterns we're using here, since those are only
> a subset of the 754 standard).

Well nominally they are all IEEE right? But yeah I think directed tests
are the answer here.

>
> So, my proposal for a v5:
>
> - Commit the test files we need, instead of downloading them from
>   the web. No whitelisting/exceptions except for tininess
>   detection, which is necessary.

Sounds good to me. Perhaps we could do a one time conversion of the test
files so they are a little more readable if we are going to own/extend
them?

>
> - fp-test is added to make test. This is a unit test of softfloat,
>   not a TCG unit test.
>
> - We defer TCG unit tests of FP to a later time.

Yeah mashing the two together is probably more trouble than it's worth.

I was playing around trying to improve the fcvt test (horribly WIP):

 https://github.com/stsquad/qemu/tree/arm/more-fcvt-tests

Anyway I'm coming to the conclusion that what we need for the TCG tests
is a generalised op tester framework that make it easy to plug in new
tests with custom inline asm with a minimal amount of fuss. I'll have a
go at this tomorrow - lets see if I can have a common framework that
abstracts away the 1, 2 and 3 source specifics and result size handling.

--
Alex Bennée