[PATCH v2 0/6] s390x/tcg: Vector instruction fixes

[PATCH v2 0/6] s390x/tcg: Vector instruction fixes

[David Hildenbrand]

Ivan reported that a simple
    $ go get -v -d github.com/FactomProject/factom

Will result in errors when vector instructions are in use. Turns out
golang makes excessive use of vector instructions, e.g., for crypto, hashes
but also basic math.

I tracked the involved vector instructions and started writing more
tests for them (will upstream them once they are in a better shape). Turns
out there are quite some issues remaining. golang uses instructions not
yet used by the kernel or by glibc.

With these patches, "go get" works again. It wouldn't surprise me if there
are more BUGs in the vector instructions. Will continue writing more tests.

Cc: Ivan Warren <ivan@vmfacility.fr>

v1 -> v2:
- "s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION"
-- Fix by inverting the result
- "s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION"
-- Reuse gen_ac2_i64()
- "s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION"
-- Reuse gen_accc2_i64()
- Ran against more tests

David Hildenbrand (6):
  s390x/tcg: Fix VECTOR MULTIPLY LOGICAL ODD
  s390x/tcg: Fix VECTOR MULTIPLY AND ADD *
  s390x/tcg: Fix VECTOR SHIFT RIGHT ARITHMETIC BY BYTE
  s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION
  s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION
  s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION

 target/s390x/translate_vx.inc.c | 30 ++++++++++++------------------
 target/s390x/vec_int_helper.c   | 18 ++++++++++--------
 2 files changed, 22 insertions(+), 26 deletions(-)

-- 
2.21.0

[PATCH v2 1/6] s390x/tcg: Fix VECTOR MULTIPLY LOGICAL ODD

[David Hildenbrand]

We have to read from odd offsets.

Fixes: 2bf3ee38f1f8 ("s390x/tcg: Implement VECTOR MULTIPLY *")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/vec_int_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
index 68eaae407b..03ae8631d9 100644
--- a/target/s390x/vec_int_helper.c
+++ b/target/s390x/vec_int_helper.c
@@ -488,7 +488,7 @@ void HELPER(gvec_vmlo##BITS)(void *v1, const void *v2, const void *v3,         \
 {                                                                              \
     int i, j;                                                                  \
                                                                                \
-    for (i = 0, j = 0; i < (128 / TBITS); i++, j += 2) {                       \
+    for (i = 0, j = 1; i < (128 / TBITS); i++, j += 2) {                       \
         const uint##TBITS##_t a = s390_vec_read_element##BITS(v2, j);          \
         const uint##TBITS##_t b = s390_vec_read_element##BITS(v3, j);          \
                                                                                \
-- 
2.21.0

[PATCH v2 2/6] s390x/tcg: Fix VECTOR MULTIPLY AND ADD *

[David Hildenbrand]

We missed that we always read a "double-wide even-odd element
pair of the fourth operand". Fix it in all four variants.

Fixes: 1b430aec4157 ("s390x/tcg: Implement VECTOR MULTIPLY AND ADD *")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/vec_int_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
index 03ae8631d9..1b3aaecbdb 100644
--- a/target/s390x/vec_int_helper.c
+++ b/target/s390x/vec_int_helper.c
@@ -336,7 +336,7 @@ void HELPER(gvec_vmae##BITS)(void *v1, const void *v2, const void *v3,         \
     for (i = 0, j = 0; i < (128 / TBITS); i++, j += 2) {                       \
         int##TBITS##_t a = (int##BITS##_t)s390_vec_read_element##BITS(v2, j);  \
         int##TBITS##_t b = (int##BITS##_t)s390_vec_read_element##BITS(v3, j);  \
-        int##TBITS##_t c = (int##BITS##_t)s390_vec_read_element##BITS(v4, j);  \
+        int##TBITS##_t c = s390_vec_read_element##TBITS(v4, i);                \
                                                                                \
         s390_vec_write_element##TBITS(v1, i, a * b + c);                       \
     }                                                                          \
@@ -354,7 +354,7 @@ void HELPER(gvec_vmale##BITS)(void *v1, const void *v2, const void *v3,        \
     for (i = 0, j = 0; i < (128 / TBITS); i++, j += 2) {                       \
         uint##TBITS##_t a = s390_vec_read_element##BITS(v2, j);                \
         uint##TBITS##_t b = s390_vec_read_element##BITS(v3, j);                \
-        uint##TBITS##_t c = s390_vec_read_element##BITS(v4, j);                \
+        uint##TBITS##_t c = s390_vec_read_element##TBITS(v4, i);               \
                                                                                \
         s390_vec_write_element##TBITS(v1, i, a * b + c);                       \
     }                                                                          \
@@ -372,7 +372,7 @@ void HELPER(gvec_vmao##BITS)(void *v1, const void *v2, const void *v3,         \
     for (i = 0, j = 1; i < (128 / TBITS); i++, j += 2) {                       \
         int##TBITS##_t a = (int##BITS##_t)s390_vec_read_element##BITS(v2, j);  \
         int##TBITS##_t b = (int##BITS##_t)s390_vec_read_element##BITS(v3, j);  \
-        int##TBITS##_t c = (int##BITS##_t)s390_vec_read_element##BITS(v4, j);  \
+        int##TBITS##_t c = s390_vec_read_element##TBITS(v4, i);                \
                                                                                \
         s390_vec_write_element##TBITS(v1, i, a * b + c);                       \
     }                                                                          \
@@ -390,7 +390,7 @@ void HELPER(gvec_vmalo##BITS)(void *v1, const void *v2, const void *v3,        \
     for (i = 0, j = 1; i < (128 / TBITS); i++, j += 2) {                       \
         uint##TBITS##_t a = s390_vec_read_element##BITS(v2, j);                \
         uint##TBITS##_t b = s390_vec_read_element##BITS(v3, j);                \
-        uint##TBITS##_t c = s390_vec_read_element##BITS(v4, j);                \
+        uint##TBITS##_t c = s390_vec_read_element##TBITS(v4, i);               \
                                                                                \
         s390_vec_write_element##TBITS(v1, i, a * b + c);                       \
     }                                                                          \
-- 
2.21.0

[PATCH v2 3/6] s390x/tcg: Fix VECTOR SHIFT RIGHT ARITHMETIC BY BYTE

[David Hildenbrand]

We forgot to propagate the highest bit accross the high doubleword in
two cases (shift >=64).

Fixes: 5f724887e3dd ("s390x/tcg: Implement VECTOR SHIFT RIGHT ARITHMETIC")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/vec_int_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
index 1b3aaecbdb..d38405848f 100644
--- a/target/s390x/vec_int_helper.c
+++ b/target/s390x/vec_int_helper.c
@@ -70,15 +70,17 @@ static void s390_vec_sar(S390Vector *d, const S390Vector *a, uint64_t count)
         d->doubleword[0] = a->doubleword[0];
         d->doubleword[1] = a->doubleword[1];
     } else if (count == 64) {
+        tmp = (int64_t)a->doubleword[0] >> 63;
         d->doubleword[1] = a->doubleword[0];
-        d->doubleword[0] = 0;
+        d->doubleword[0] = tmp;
     } else if (count < 64) {
         tmp = a->doubleword[1] >> count;
         d->doubleword[1] = deposit64(tmp, 64 - count, count, a->doubleword[0]);
         d->doubleword[0] = (int64_t)a->doubleword[0] >> count;
     } else {
+        tmp = (int64_t)a->doubleword[0] >> 63;
         d->doubleword[1] = (int64_t)a->doubleword[0] >> (count - 64);
-        d->doubleword[0] = 0;
+        d->doubleword[0] = tmp;
     }
 }
 
-- 
2.21.0

[PATCH v2 4/6] s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION

[David Hildenbrand]

Looks like my idea of what a "borrow" is was wrong. The PoP says:

 "If the resulting subtraction results in a carry out of bit zero, a value
 of one is placed in the corresponding element of the first operand;
 otherwise, a value of zero is placed in the corresponding element"

As clarified by Richard, all we have to do is invert the result.

Fixes: 1ee2d7ba72f6 ("s390x/tcg: Implement VECTOR SUBTRACT COMPUTE BORROW INDICATION")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/translate_vx.inc.c | 7 ++++---
 target/s390x/vec_int_helper.c   | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 5ce7bfb0af..6032021d82 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2132,12 +2132,12 @@ static DisasJumpType op_vs(DisasContext *s, DisasOps *o)
 
 static void gen_scbi_i32(TCGv_i32 d, TCGv_i32 a, TCGv_i32 b)
 {
-    tcg_gen_setcond_i32(TCG_COND_LTU, d, a, b);
+    tcg_gen_setcond_i32(TCG_COND_GEU, d, a, b);
 }
 
 static void gen_scbi_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
 {
-    tcg_gen_setcond_i64(TCG_COND_LTU, d, a, b);
+    tcg_gen_setcond_i64(TCG_COND_GEU, d, a, b);
 }
 
 static void gen_scbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al,
@@ -2151,7 +2151,8 @@ static void gen_scbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al,
     tcg_gen_andi_i64(th, th, 1);
     tcg_gen_sub2_i64(tl, th, ah, zero, th, zero);
     tcg_gen_sub2_i64(tl, th, tl, th, bh, zero);
-    tcg_gen_andi_i64(dl, th, 1);
+    /* "invert" the result: -1 -> 0; 0 -> 1 */
+    tcg_gen_addi_i64(dl, th, 1);
     tcg_gen_mov_i64(dh, zero);
 
     tcg_temp_free_i64(th);
diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
index d38405848f..0d6bc13dd6 100644
--- a/target/s390x/vec_int_helper.c
+++ b/target/s390x/vec_int_helper.c
@@ -593,7 +593,7 @@ void HELPER(gvec_vscbi##BITS)(void *v1, const void *v2, const void *v3,        \
         const uint##BITS##_t a = s390_vec_read_element##BITS(v2, i);           \
         const uint##BITS##_t b = s390_vec_read_element##BITS(v3, i);           \
                                                                                \
-        s390_vec_write_element##BITS(v1, i, a < b);                            \
+        s390_vec_write_element##BITS(v1, i, a >= b);                           \
     }                                                                          \
 }
 DEF_VSCBI(8)
-- 
2.21.0

Re: [PATCH v2 4/6] s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION

[Richard Henderson]

On 10/21/19 1:57 AM, David Hildenbrand wrote:
> Looks like my idea of what a "borrow" is was wrong. The PoP says:
> 
>  "If the resulting subtraction results in a carry out of bit zero, a value
>  of one is placed in the corresponding element of the first operand;
>  otherwise, a value of zero is placed in the corresponding element"
> 
> As clarified by Richard, all we have to do is invert the result.
> 
> Fixes: 1ee2d7ba72f6 ("s390x/tcg: Implement VECTOR SUBTRACT COMPUTE BORROW INDICATION")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/translate_vx.inc.c | 7 ++++---
>  target/s390x/vec_int_helper.c   | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH v2 5/6] s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION

[David Hildenbrand]

Testing this, there seems to be something messed up. We are dealing with
unsigned numbers. "Each operand is treated as an unsigned binary integer."
Let's just implement as written in the PoP:

"A subtraction is performed by adding the contents of
 the second operand with the bitwise complement of
 the third operand along with a borrow indication from
 the rightmost bit position of the fourth operand and
 the result is placed in the first operand."

We can reuse gen_ac2_i64().

Fixes: 48390a7c2716 ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW INDICATION")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/translate_vx.inc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 6032021d82..fd746ba35f 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2187,13 +2187,13 @@ static void gen_sbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al, TCGv_i64 ah,
                          TCGv_i64 bl, TCGv_i64 bh, TCGv_i64 cl, TCGv_i64 ch)
 {
     TCGv_i64 tl = tcg_temp_new_i64();
-    TCGv_i64 zero = tcg_const_i64(0);
+    TCGv_i64 th = tcg_temp_new_i64();
 
-    tcg_gen_andi_i64(tl, cl, 1);
-    tcg_gen_sub2_i64(dl, dh, al, ah, bl, bh);
-    tcg_gen_sub2_i64(dl, dh, dl, dh, tl, zero);
+    tcg_gen_not_i64(tl, bl);
+    tcg_gen_not_i64(th, bh);
+    gen_ac2_i64(dl, dh, al, ah, tl, th, cl, ch);
     tcg_temp_free_i64(tl);
-    tcg_temp_free_i64(zero);
+    tcg_temp_free_i64(th);
 }
 
 static DisasJumpType op_vsbi(DisasContext *s, DisasOps *o)
-- 
2.21.0

Re: [PATCH v2 5/6] s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION

[Richard Henderson]

On 10/21/19 1:57 AM, David Hildenbrand wrote:
> Testing this, there seems to be something messed up. We are dealing with
> unsigned numbers. "Each operand is treated as an unsigned binary integer."
> Let's just implement as written in the PoP:
> 
> "A subtraction is performed by adding the contents of
>  the second operand with the bitwise complement of
>  the third operand along with a borrow indication from
>  the rightmost bit position of the fourth operand and
>  the result is placed in the first operand."
> 
> We can reuse gen_ac2_i64().
> 
> Fixes: 48390a7c2716 ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW INDICATION")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/translate_vx.inc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH v2 6/6] s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION

[David Hildenbrand]

The numbers are unsigned, the computation is wrong. "Each operand is
treated as an unsigned binary integer".
Let's implement as given in the PoP:

"A subtraction is performed by adding the contents of the second operand
 with the bitwise complement of the third operand along with a borrow
 indication from the rightmost bit of the fourth operand."

Reuse gen_accc2_i64().

Fixes: bc725e65152c ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/translate_vx.inc.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index fd746ba35f..71059f9ca0 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2214,20 +2214,13 @@ static void gen_sbcbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al, TCGv_i64 ah,
 {
     TCGv_i64 th = tcg_temp_new_i64();
     TCGv_i64 tl = tcg_temp_new_i64();
-    TCGv_i64 zero = tcg_const_i64(0);
 
-    tcg_gen_andi_i64(tl, cl, 1);
-    tcg_gen_sub2_i64(tl, th, al, zero, tl, zero);
-    tcg_gen_sub2_i64(tl, th, tl, th, bl, zero);
-    tcg_gen_andi_i64(th, th, 1);
-    tcg_gen_sub2_i64(tl, th, ah, zero, th, zero);
-    tcg_gen_sub2_i64(tl, th, tl, th, bh, zero);
-    tcg_gen_andi_i64(dl, th, 1);
-    tcg_gen_mov_i64(dh, zero);
+    tcg_gen_not_i64(tl, bl);
+    tcg_gen_not_i64(th, bh);
+    gen_accc2_i64(dl, dh, al, ah, tl, th, cl, ch);
 
     tcg_temp_free_i64(tl);
     tcg_temp_free_i64(th);
-    tcg_temp_free_i64(zero);
 }
 
 static DisasJumpType op_vsbcbi(DisasContext *s, DisasOps *o)
-- 
2.21.0

Re: [PATCH v2 6/6] s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION

[Richard Henderson]

On 10/21/19 1:57 AM, David Hildenbrand wrote:
> The numbers are unsigned, the computation is wrong. "Each operand is
> treated as an unsigned binary integer".
> Let's implement as given in the PoP:
> 
> "A subtraction is performed by adding the contents of the second operand
>  with the bitwise complement of the third operand along with a borrow
>  indication from the rightmost bit of the fourth operand."
> 
> Reuse gen_accc2_i64().
> 
> Fixes: bc725e65152c ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/translate_vx.inc.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 0/6] s390x/tcg: Vector instruction fixes

[Cornelia Huck]

On Mon, 21 Oct 2019 10:57:09 +0200
David Hildenbrand <david@redhat.com> wrote:

> Ivan reported that a simple
>     $ go get -v -d github.com/FactomProject/factom
> 
> Will result in errors when vector instructions are in use. Turns out
> golang makes excessive use of vector instructions, e.g., for crypto, hashes
> but also basic math.
> 
> I tracked the involved vector instructions and started writing more
> tests for them (will upstream them once they are in a better shape). Turns
> out there are quite some issues remaining. golang uses instructions not
> yet used by the kernel or by glibc.
> 
> With these patches, "go get" works again. It wouldn't surprise me if there
> are more BUGs in the vector instructions. Will continue writing more tests.
> 
> Cc: Ivan Warren <ivan@vmfacility.fr>
> 
> v1 -> v2:
> - "s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION"
> -- Fix by inverting the result
> - "s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION"
> -- Reuse gen_ac2_i64()
> - "s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION"
> -- Reuse gen_accc2_i64()
> - Ran against more tests
> 
> David Hildenbrand (6):
>   s390x/tcg: Fix VECTOR MULTIPLY LOGICAL ODD
>   s390x/tcg: Fix VECTOR MULTIPLY AND ADD *
>   s390x/tcg: Fix VECTOR SHIFT RIGHT ARITHMETIC BY BYTE
>   s390x/tcg: Fix VECTOR SUBTRACT COMPUTE BORROW INDICATION
>   s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW INDICATION
>   s390x/tcg: Fix VECTOR SUBTRACT WITH BORROW COMPUTE BORROW INDICATION
> 
>  target/s390x/translate_vx.inc.c | 30 ++++++++++++------------------
>  target/s390x/vec_int_helper.c   | 18 ++++++++++--------
>  2 files changed, 22 insertions(+), 26 deletions(-)
> 

Thanks, applied.