mbox series

[v1,00/20] s390x/tcg: Implement Vector enhancements facility and switch to z14

Message ID 20200930145523.71087-1-david@redhat.com
Headers show
Series s390x/tcg: Implement Vector enhancements facility and switch to z14 | expand

Message

David Hildenbrand Sept. 30, 2020, 2:55 p.m. UTC 
This series adds support for the "Vector enhancements facility" and bumps
the qemu CPU model tp to a stripped-down z14.

I yet have to find a way to get more test coverage - looks like some of
the functions aren't used anywhere yet (e.g., VECTOR FP MAXIMUM), writing
unit tests to cover all functions and cases is just nasty. But I might be
wrong - I'm planning to at least test basic functionality of all new added
instructions.

I have to make excessive use of c macros again to cover different element
sizes (32/64/128bit). Any advise to clean things up are welcome.

This is based on:
    "[PATCH v2 0/9] s390x/tcg: Implement some z14 facilities"
    "[PATCH v2 00/10] softfloat: Implement float128_muladd"

Based-on: 20200928122717.30586-1-david@redhat.com
Based-on: 20200925152047.709901-1-richard.henderson@linaro.org

David Hildenbrand (20):
  softfloat: Implement
    float128_(min|minnum|minnummag|max|maxnum|maxnummag)
  s390x/tcg: Implement VECTOR BIT PERMUTE
  s390x/tcg: Implement VECTOR MULTIPLY SUM LOGICAL
  s390x/tcg: Implement 32/128 bit for VECTOR FP ADD
  s390x/tcg: Implement 32/128 bit for VECTOR FP DIVIDE
  s390x/tcg: Implement 32/128 bit for VECTOR FP MULTIPLY
  s390x/tcg: Implement 32/128 bit for VECTOR FP SUBTRACT
  s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE (AND SIGNAL)
    SCALAR
  s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE *
  s390x/tcg: Implement 32/128 bit for VECTOR LOAD FP INTEGER
  s390x/tcg: Implement 64 bit for VECTOR FP LOAD LENGTHENED
  s390x/tcg: Implement 128 bit for VECTOR FP LOAD ROUNDED
  s390x/tcg: Implement 32/128 bit for VECTOR FP PERFORM SIGN OPERATION
  s390x/tcg: Implement 32/128 bit for VECTOR FP SQUARE ROOT
  s390x/tcg: Implement 32/128 bit for VECTOR FP TEST DATA CLASS
    IMMEDIATE
  s390x/tcg: Implement 32/128bit for VECTOR FP MULTIPLY AND
    (ADD|SUBTRACT)
  s390x/tcg: Implement VECTOR FP NEGATIVE MULTIPLY AND (ADD|SUBTRACT)
  s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM)
  s390x/tcg: We support Vector enhancements facility
  s390x/cpumodel: Bump up QEMU model to a stripped-down IBM z14 GA2

 fpu/softfloat.c                 |  100 +++
 hw/s390x/s390-virtio-ccw.c      |    2 +
 include/fpu/softfloat.h         |    6 +
 target/s390x/cpu_models.c       |    4 +-
 target/s390x/gen-features.c     |   14 +-
 target/s390x/helper.h           |   72 ++
 target/s390x/insn-data.def      |   12 +
 target/s390x/translate_vx.c.inc |  625 ++++++++++++---
 target/s390x/vec_fpu_helper.c   | 1302 ++++++++++++++++++++++---------
 target/s390x/vec_helper.c       |   22 +
 10 files changed, 1681 insertions(+), 478 deletions(-)

Comments

no-reply@patchew.org Sept. 30, 2020, 3:35 p.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20200930145523.71087-1-david@redhat.com/



Hi,

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

Type: series
Message-id: 20200930145523.71087-1-david@redhat.com
Subject: [PATCH v1 00/20] s390x/tcg: Implement Vector enhancements facility and switch to z14

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
855b893 s390x/cpumodel: Bump up QEMU model to a stripped-down IBM z14 GA2
9e2c5a3 s390x/tcg: We support Vector enhancements facility
91355af s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM)
22ae891 s390x/tcg: Implement VECTOR FP NEGATIVE MULTIPLY AND (ADD|SUBTRACT)
599e1d8 s390x/tcg: Implement 32/128bit for VECTOR FP MULTIPLY AND (ADD|SUBTRACT)
2d46f63 s390x/tcg: Implement 32/128 bit for VECTOR FP TEST DATA CLASS IMMEDIATE
7f46bf1 s390x/tcg: Implement 32/128 bit for VECTOR FP SQUARE ROOT
0870741 s390x/tcg: Implement 32/128 bit for VECTOR FP PERFORM SIGN OPERATION
743a4a3 s390x/tcg: Implement 128 bit for VECTOR FP LOAD ROUNDED
793d28c s390x/tcg: Implement 64 bit for VECTOR FP LOAD LENGTHENED
ff85384 s390x/tcg: Implement 32/128 bit for VECTOR LOAD FP INTEGER
de00280 s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE *
ba7aec7 s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE (AND SIGNAL) SCALAR
74af73a s390x/tcg: Implement 32/128 bit for VECTOR FP SUBTRACT
59a6f7a s390x/tcg: Implement 32/128 bit for VECTOR FP MULTIPLY
c0903b4 s390x/tcg: Implement 32/128 bit for VECTOR FP DIVIDE
8af7fca s390x/tcg: Implement 32/128 bit for VECTOR FP ADD
e103776 s390x/tcg: Implement VECTOR MULTIPLY SUM LOGICAL
092690a s390x/tcg: Implement VECTOR BIT PERMUTE
46d60ed softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag)

=== OUTPUT BEGIN ===
1/20 Checking commit 46d60ede64da (softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag))
2/20 Checking commit 092690a01e4b (s390x/tcg: Implement VECTOR BIT PERMUTE)
3/20 Checking commit e1037765c06f (s390x/tcg: Implement VECTOR MULTIPLY SUM LOGICAL)
4/20 Checking commit 8af7fcaee5ef (s390x/tcg: Implement 32/128 bit for VECTOR FP ADD)
ERROR: space prohibited between function name and open parenthesis '('
#162: FILE: target/s390x/vec_fpu_helper.c:140:
+typedef float##BITS (*vop##BITS##_3_fn)(float##BITS a, float##BITS b,          \

total: 1 errors, 0 warnings, 183 lines checked

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

5/20 Checking commit c0903b4e139a (s390x/tcg: Implement 32/128 bit for VECTOR FP DIVIDE)
6/20 Checking commit 59a6f7a3d4ca (s390x/tcg: Implement 32/128 bit for VECTOR FP MULTIPLY)
7/20 Checking commit 74af73a36417 (s390x/tcg: Implement 32/128 bit for VECTOR FP SUBTRACT)
8/20 Checking commit ba7aec76b6d7 (s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE (AND SIGNAL) SCALAR)
WARNING: Block comments use a leading /* on a separate line
#113: FILE: target/s390x/vec_fpu_helper.c:190:
+    /* only the zero-indexed elements are compared */                          \

total: 0 errors, 1 warnings, 136 lines checked

Patch 8/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/20 Checking commit de00280993e0 (s390x/tcg: Implement 32/128 bit for VECTOR FP COMPARE *)
WARNING: line over 80 characters
#26: FILE: target/s390x/helper.h:262:
+DEF_HELPER_FLAGS_5(gvec_vfce32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#29: FILE: target/s390x/helper.h:265:
+DEF_HELPER_FLAGS_5(gvec_vfce128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#36: FILE: target/s390x/helper.h:272:
+DEF_HELPER_FLAGS_5(gvec_vfch32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#39: FILE: target/s390x/helper.h:275:
+DEF_HELPER_FLAGS_5(gvec_vfch128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#45: FILE: target/s390x/helper.h:281:
+DEF_HELPER_FLAGS_5(gvec_vfche32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#46: FILE: target/s390x/helper.h:282:
+DEF_HELPER_FLAGS_5(gvec_vfche32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#49: FILE: target/s390x/helper.h:285:
+DEF_HELPER_FLAGS_5(gvec_vfche128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: Block comments use a leading /* on a separate line
#250: FILE: target/s390x/vec_fpu_helper.c:250:
+        /* swap the parameters, so we can use existing functions */            \

total: 0 errors, 8 warnings, 445 lines checked

Patch 9/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/20 Checking commit ff85384a58d4 (s390x/tcg: Implement 32/128 bit for VECTOR LOAD FP INTEGER)
ERROR: space prohibited between function name and open parenthesis '('
#140: FILE: target/s390x/vec_fpu_helper.c:120:
+typedef float##BITS (*vop##BITS##_2_fn)(float##BITS a, float_status *s);       \

total: 1 errors, 0 warnings, 191 lines checked

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

11/20 Checking commit 793d28cefc57 (s390x/tcg: Implement 64 bit for VECTOR FP LOAD LENGTHENED)
12/20 Checking commit 743a4a3474e5 (s390x/tcg: Implement 128 bit for VECTOR FP LOAD ROUNDED)
13/20 Checking commit 08707417ca0e (s390x/tcg: Implement 32/128 bit for VECTOR FP PERFORM SIGN OPERATION)
14/20 Checking commit 7f46bf12ce3e (s390x/tcg: Implement 32/128 bit for VECTOR FP SQUARE ROOT)
15/20 Checking commit 2d46f639d6dd (s390x/tcg: Implement 32/128 bit for VECTOR FP TEST DATA CLASS IMMEDIATE)
16/20 Checking commit 599e1d8a08ff (s390x/tcg: Implement 32/128bit for VECTOR FP MULTIPLY AND (ADD|SUBTRACT))
WARNING: line over 80 characters
#20: FILE: target/s390x/helper.h:320:
+DEF_HELPER_FLAGS_6(gvec_vfma32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#21: FILE: target/s390x/helper.h:321:
+DEF_HELPER_FLAGS_6(gvec_vfma32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#24: FILE: target/s390x/helper.h:324:
+DEF_HELPER_FLAGS_6(gvec_vfma128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#25: FILE: target/s390x/helper.h:325:
+DEF_HELPER_FLAGS_6(gvec_vfms32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#26: FILE: target/s390x/helper.h:326:
+DEF_HELPER_FLAGS_6(gvec_vfms32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#29: FILE: target/s390x/helper.h:329:
+DEF_HELPER_FLAGS_6(gvec_vfms128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

total: 0 errors, 6 warnings, 188 lines checked

Patch 16/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
17/20 Checking commit 22ae891fa9a5 (s390x/tcg: Implement VECTOR FP NEGATIVE MULTIPLY AND (ADD|SUBTRACT))
WARNING: line over 80 characters
#20: FILE: target/s390x/helper.h:330:
+DEF_HELPER_FLAGS_6(gvec_vfnma32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#21: FILE: target/s390x/helper.h:331:
+DEF_HELPER_FLAGS_6(gvec_vfnma32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#22: FILE: target/s390x/helper.h:332:
+DEF_HELPER_FLAGS_6(gvec_vfnma64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#23: FILE: target/s390x/helper.h:333:
+DEF_HELPER_FLAGS_6(gvec_vfnma64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#24: FILE: target/s390x/helper.h:334:
+DEF_HELPER_FLAGS_6(gvec_vfnma128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#25: FILE: target/s390x/helper.h:335:
+DEF_HELPER_FLAGS_6(gvec_vfnms32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#26: FILE: target/s390x/helper.h:336:
+DEF_HELPER_FLAGS_6(gvec_vfnms32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#27: FILE: target/s390x/helper.h:337:
+DEF_HELPER_FLAGS_6(gvec_vfnms64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#28: FILE: target/s390x/helper.h:338:
+DEF_HELPER_FLAGS_6(gvec_vfnms64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#29: FILE: target/s390x/helper.h:339:
+DEF_HELPER_FLAGS_6(gvec_vfnms128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, cptr, env, i32)

total: 0 errors, 10 warnings, 109 lines checked

Patch 17/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
18/20 Checking commit 91355af73073 (s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM))
WARNING: line over 80 characters
#28: FILE: target/s390x/helper.h:320:
+DEF_HELPER_FLAGS_5(gvec_vfmax32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#29: FILE: target/s390x/helper.h:321:
+DEF_HELPER_FLAGS_5(gvec_vfmax32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#30: FILE: target/s390x/helper.h:322:
+DEF_HELPER_FLAGS_5(gvec_vfmax64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#31: FILE: target/s390x/helper.h:323:
+DEF_HELPER_FLAGS_5(gvec_vfmax64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#32: FILE: target/s390x/helper.h:324:
+DEF_HELPER_FLAGS_5(gvec_vfmax128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#33: FILE: target/s390x/helper.h:325:
+DEF_HELPER_FLAGS_5(gvec_vfmin32s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#34: FILE: target/s390x/helper.h:326:
+DEF_HELPER_FLAGS_5(gvec_vfmin32, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#35: FILE: target/s390x/helper.h:327:
+DEF_HELPER_FLAGS_5(gvec_vfmin64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#36: FILE: target/s390x/helper.h:328:
+DEF_HELPER_FLAGS_5(gvec_vfmin64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: line over 80 characters
#37: FILE: target/s390x/helper.h:329:
+DEF_HELPER_FLAGS_5(gvec_vfmin128, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)

WARNING: Block comments use a leading /* on a separate line
#158: FILE: target/s390x/vec_fpu_helper.c:941:
+            /* fall through */                                                 \

WARNING: Block comments use a leading /* on a separate line
#211: FILE: target/s390x/vec_fpu_helper.c:994:
+    /* We can process all remaining cases using simple comparison. */          \

total: 0 errors, 12 warnings, 379 lines checked

Patch 18/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
19/20 Checking commit 9e2c5a3530bd (s390x/tcg: We support Vector enhancements facility)
20/20 Checking commit 855b893d0a4d (s390x/cpumodel: Bump up QEMU model to a stripped-down IBM z14 GA2)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200930145523.71087-1-david@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Richard Henderson Oct. 1, 2020, 3:07 p.m. UTC | #2 
On 9/30/20 9:55 AM, David Hildenbrand wrote:
> This series adds support for the "Vector enhancements facility" and bumps

> the qemu CPU model tp to a stripped-down z14.

> 

> I yet have to find a way to get more test coverage - looks like some of

> the functions aren't used anywhere yet (e.g., VECTOR FP MAXIMUM), writing

> unit tests to cover all functions and cases is just nasty. But I might be

> wrong - I'm planning to at least test basic functionality of all new added

> instructions.


This is where RISU can be helpful.  Auto-generate 100k random variations,
record known good results on hardware, verify that replay on qemu produces the
same results.


r~
David Hildenbrand Oct. 7, 2020, 1:09 p.m. UTC | #3 
On 01.10.20 17:07, Richard Henderson wrote:
> On 9/30/20 9:55 AM, David Hildenbrand wrote:

>> This series adds support for the "Vector enhancements facility" and bumps

>> the qemu CPU model tp to a stripped-down z14.

>>

>> I yet have to find a way to get more test coverage - looks like some of

>> the functions aren't used anywhere yet (e.g., VECTOR FP MAXIMUM), writing

>> unit tests to cover all functions and cases is just nasty. But I might be

>> wrong - I'm planning to at least test basic functionality of all new added

>> instructions.

> 

> This is where RISU can be helpful.  Auto-generate 100k random variations,

> record known good results on hardware, verify that replay on qemu produces the

> same results.


Makes sense, however, some corner cases (e.g., MAX(+0, -O)) still might
have to be handled manually.

It might take me a while to address the feedback (I'm  fairly busy as
usual ...). Thanks!

-- 
Thanks,

David / dhildenb
David Hildenbrand May 5, 2021, 10:55 a.m. UTC | #4 
On 30.09.20 16:55, David Hildenbrand wrote:
> This series adds support for the "Vector enhancements facility" and bumps

> the qemu CPU model tp to a stripped-down z14.

> 

> I yet have to find a way to get more test coverage - looks like some of

> the functions aren't used anywhere yet (e.g., VECTOR FP MAXIMUM), writing

> unit tests to cover all functions and cases is just nasty. But I might be

> wrong - I'm planning to at least test basic functionality of all new added

> instructions.

> 

> I have to make excessive use of c macros again to cover different element

> sizes (32/64/128bit). Any advise to clean things up are welcome.

> 

> This is based on:

>      "[PATCH v2 0/9] s390x/tcg: Implement some z14 facilities"

>      "[PATCH v2 00/10] softfloat: Implement float128_muladd"

> 

> Based-on: 20200928122717.30586-1-david@redhat.com

> Based-on: 20200925152047.709901-1-richard.henderson@linaro.org


Hi Richard,

I'll have a new series ready soonish. I did what you suggested and 
started generating random (valid) Vector FP instructions with (valid) 
random parameters, executed on randomly generated vectors. It's looking 
pretty good by now.

I'll still have to see to which degree I can address feedback on 
"softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag)"

Anyhow, I'd need your "softfloat: Implement float128_muladd" series -- 
any idea when you might get to continue working on that? Thanks!

-- 
Thanks,

David / dhildenb