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

[PATCH v1 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>

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/helper.h           |  2 -
 target/s390x/translate_vx.inc.c | 78 +++++++++++++++++++++++++--------
 target/s390x/vec_int_helper.c   | 32 ++++----------
 3 files changed, 68 insertions(+), 44 deletions(-)

-- 
2.21.0

[PATCH v1 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 *")
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

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

[Richard Henderson]

On 10/18/19 9:10 AM, David Hildenbrand wrote:
> We have to read from odd offsets.
> 
> Fixes: 2bf3ee38f1f8 ("s390x/tcg: Implement VECTOR MULTIPLY *")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/vec_int_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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


r~

[PATCH v1 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 *")
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

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

[Richard Henderson]

On 10/18/19 9:10 AM, David Hildenbrand wrote:
> 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 *")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/vec_int_helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

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


r~

[PATCH v1 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")
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

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

[Richard Henderson]

On 10/18/19 9:10 AM, David Hildenbrand wrote:
> 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")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/vec_int_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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


r~

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

[David Hildenbrand]

Looks like my idea of what a "borrow" is was wrong. We are dealing with
unsigned numbers. A subtraction is simply an addition with the bitwise
complement. If we get a carry during the addition, that's the borrow.
"The operands are treated as unsigned binary integers."

This is nice, as we can reuse the VECTOR ADD COMPUTE CARRY functions
and avoid helpers, all we have to do is compute the bitwise complement.

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

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 56e8149866..ca1e08100a 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -207,8 +207,6 @@ DEF_HELPER_FLAGS_4(gvec_verim16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
 DEF_HELPER_FLAGS_4(gvec_vsl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
 DEF_HELPER_FLAGS_4(gvec_vsra, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
 DEF_HELPER_FLAGS_4(gvec_vsrl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
-DEF_HELPER_FLAGS_4(gvec_vscbi8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
-DEF_HELPER_FLAGS_4(gvec_vscbi16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
 DEF_HELPER_4(gvec_vtm, void, ptr, cptr, env, i32)
 
 /* === Vector String Instructions === */
diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 5ce7bfb0af..40bcc1604e 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2130,14 +2130,40 @@ static DisasJumpType op_vs(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
+static void gen_scbi8_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
+{
+    TCGv_i64 t = tcg_temp_new_i64();
+
+    tcg_gen_not_i64(t, b);
+    gen_acc(d, a, t, ES_8);
+    tcg_temp_free_i64(t);
+}
+
+static void gen_scbi16_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
+{
+    TCGv_i64 t = tcg_temp_new_i64();
+
+    tcg_gen_not_i64(t, b);
+    gen_acc(d, a, t, ES_16);
+    tcg_temp_free_i64(t);
+}
+
 static void gen_scbi_i32(TCGv_i32 d, TCGv_i32 a, TCGv_i32 b)
 {
-    tcg_gen_setcond_i32(TCG_COND_LTU, d, a, b);
+    TCGv_i32 t = tcg_temp_new_i32();
+
+    tcg_gen_not_i32(t, b);
+    gen_acc_i32(d, a, t);
+    tcg_temp_free_i32(t);
 }
 
 static void gen_scbi_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
 {
-    tcg_gen_setcond_i64(TCG_COND_LTU, d, a, b);
+    TCGv_i64 t = tcg_temp_new_i64();
+
+    tcg_gen_not_i64(t, b);
+    gen_acc_i64(d, a, t);
+    tcg_temp_free_i64(t);
 }
 
 static void gen_scbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al,
@@ -2145,26 +2171,21 @@ static void gen_scbi2_i64(TCGv_i64 dl, TCGv_i64 dh, TCGv_i64 al,
 {
     TCGv_i64 th = tcg_temp_new_i64();
     TCGv_i64 tl = tcg_temp_new_i64();
-    TCGv_i64 zero = tcg_const_i64(0);
 
-    tcg_gen_sub2_i64(tl, th, al, zero, 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_acc2_i64(dl, dh, al, ah, tl, th);
 
     tcg_temp_free_i64(th);
     tcg_temp_free_i64(tl);
-    tcg_temp_free_i64(zero);
 }
 
 static DisasJumpType op_vscbi(DisasContext *s, DisasOps *o)
 {
     const uint8_t es = get_field(s->fields, m4);
     static const GVecGen3 g[4] = {
-        { .fno = gen_helper_gvec_vscbi8, },
-        { .fno = gen_helper_gvec_vscbi16, },
+        { .fni8 = gen_scbi8_i64, },
+        { .fni8 = gen_scbi16_i64, },
         { .fni4 = gen_scbi_i32, },
         { .fni8 = gen_scbi_i64, },
     };
diff --git a/target/s390x/vec_int_helper.c b/target/s390x/vec_int_helper.c
index d38405848f..e8fe7af496 100644
--- a/target/s390x/vec_int_helper.c
+++ b/target/s390x/vec_int_helper.c
@@ -583,22 +583,6 @@ void HELPER(gvec_vsrl)(void *v1, const void *v2, uint64_t count,
     s390_vec_shr(v1, v2, count);
 }
 
-#define DEF_VSCBI(BITS)                                                        \
-void HELPER(gvec_vscbi##BITS)(void *v1, const void *v2, const void *v3,        \
-                              uint32_t desc)                                   \
-{                                                                              \
-    int i;                                                                     \
-                                                                               \
-    for (i = 0; i < (128 / BITS); i++) {                                       \
-        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);                            \
-    }                                                                          \
-}
-DEF_VSCBI(8)
-DEF_VSCBI(16)
-
 void HELPER(gvec_vtm)(void *v1, const void *v2, CPUS390XState *env,
                       uint32_t desc)
 {
-- 
2.21.0

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

[David Hildenbrand]

On 18.10.19 18:10, David Hildenbrand wrote:
> Looks like my idea of what a "borrow" is was wrong. We are dealing with
> unsigned numbers. A subtraction is simply an addition with the bitwise
> complement. If we get a carry during the addition, that's the borrow.
> "The operands are treated as unsigned binary integers."
> 
> This is nice, as we can reuse the VECTOR ADD COMPUTE CARRY functions
> and avoid helpers, all we have to do is compute the bitwise complement.
> 
> Fixes: 1ee2d7ba72f6 ("s390x/tcg: Implement VECTOR SUBTRACT COMPUTE BORROW INDICATION")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/helper.h           |  2 --
>   target/s390x/translate_vx.inc.c | 45 ++++++++++++++++++++++++---------
>   target/s390x/vec_int_helper.c   | 16 ------------
>   3 files changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 56e8149866..ca1e08100a 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -207,8 +207,6 @@ DEF_HELPER_FLAGS_4(gvec_verim16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>   DEF_HELPER_FLAGS_4(gvec_vsl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>   DEF_HELPER_FLAGS_4(gvec_vsra, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>   DEF_HELPER_FLAGS_4(gvec_vsrl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
> -DEF_HELPER_FLAGS_4(gvec_vscbi8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
> -DEF_HELPER_FLAGS_4(gvec_vscbi16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>   DEF_HELPER_4(gvec_vtm, void, ptr, cptr, env, i32)
>   
>   /* === Vector String Instructions === */
> diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
> index 5ce7bfb0af..40bcc1604e 100644
> --- a/target/s390x/translate_vx.inc.c
> +++ b/target/s390x/translate_vx.inc.c
> @@ -2130,14 +2130,40 @@ static DisasJumpType op_vs(DisasContext *s, DisasOps *o)
>       return DISAS_NEXT;
>   }
>   
> +static void gen_scbi8_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
> +{
> +    TCGv_i64 t = tcg_temp_new_i64();
> +
> +    tcg_gen_not_i64(t, b);
> +    gen_acc(d, a, t, ES_8);
> +    tcg_temp_free_i64(t);
> +}

BTW, I would have thought that we need the 2nd complement in all these 
cases. However, the description of the other functions confused me 
(VECTOR SUBTRACT WITH BORROW INDICATION) - add bitwise complement and 
add the borrow.

This passes my test cases (that are verified against real HW), but I am 
not sure if I check all the corner cases.

@Richard, do you have any idea how to do it the right way for this 
instruction?

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 18.10.19 19:41, David Hildenbrand wrote:
> On 18.10.19 18:10, David Hildenbrand wrote:
>> Looks like my idea of what a "borrow" is was wrong. We are dealing with
>> unsigned numbers. A subtraction is simply an addition with the bitwise
>> complement. If we get a carry during the addition, that's the borrow.
>> "The operands are treated as unsigned binary integers."
>>
>> This is nice, as we can reuse the VECTOR ADD COMPUTE CARRY functions
>> and avoid helpers, all we have to do is compute the bitwise complement.
>>
>> Fixes: 1ee2d7ba72f6 ("s390x/tcg: Implement VECTOR SUBTRACT COMPUTE BORROW INDICATION")
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    target/s390x/helper.h           |  2 --
>>    target/s390x/translate_vx.inc.c | 45 ++++++++++++++++++++++++---------
>>    target/s390x/vec_int_helper.c   | 16 ------------
>>    3 files changed, 33 insertions(+), 30 deletions(-)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 56e8149866..ca1e08100a 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -207,8 +207,6 @@ DEF_HELPER_FLAGS_4(gvec_verim16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>>    DEF_HELPER_FLAGS_4(gvec_vsl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>>    DEF_HELPER_FLAGS_4(gvec_vsra, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>>    DEF_HELPER_FLAGS_4(gvec_vsrl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>> -DEF_HELPER_FLAGS_4(gvec_vscbi8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>> -DEF_HELPER_FLAGS_4(gvec_vscbi16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>>    DEF_HELPER_4(gvec_vtm, void, ptr, cptr, env, i32)
>>    
>>    /* === Vector String Instructions === */
>> diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
>> index 5ce7bfb0af..40bcc1604e 100644
>> --- a/target/s390x/translate_vx.inc.c
>> +++ b/target/s390x/translate_vx.inc.c
>> @@ -2130,14 +2130,40 @@ static DisasJumpType op_vs(DisasContext *s, DisasOps *o)
>>        return DISAS_NEXT;
>>    }
>>    
>> +static void gen_scbi8_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
>> +{
>> +    TCGv_i64 t = tcg_temp_new_i64();
>> +
>> +    tcg_gen_not_i64(t, b);
>> +    gen_acc(d, a, t, ES_8);
>> +    tcg_temp_free_i64(t);
>> +}
> 
> BTW, I would have thought that we need the 2nd complement in all these
> cases. However, the description of the other functions confused me
> (VECTOR SUBTRACT WITH BORROW INDICATION) - add bitwise complement and
> add the borrow.
> 
> This passes my test cases (that are verified against real HW), but I am
> not sure if I check all the corner cases.
> 
> @Richard, do you have any idea how to do it the right way for this
> instruction?
> 

My impression was right. A simple "0-0" test makes this visible. The 
other two fixes seem to be correct, though.

Will rework.

-- 

Thanks,

David / dhildenb

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

[Richard Henderson]

On 10/18/19 11:18 AM, David Hildenbrand wrote:
> On 18.10.19 19:41, David Hildenbrand wrote:
>> On 18.10.19 18:10, David Hildenbrand wrote:
>>> Looks like my idea of what a "borrow" is was wrong. We are dealing with
>>> unsigned numbers. A subtraction is simply an addition with the bitwise
>>> complement. If we get a carry during the addition, that's the borrow.
>>> "The operands are treated as unsigned binary integers."
>>>
>>> This is nice, as we can reuse the VECTOR ADD COMPUTE CARRY functions
>>> and avoid helpers, all we have to do is compute the bitwise complement.
>>>
>>> Fixes: 1ee2d7ba72f6 ("s390x/tcg: Implement VECTOR SUBTRACT COMPUTE BORROW
>>> INDICATION")
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    target/s390x/helper.h           |  2 --
>>>    target/s390x/translate_vx.inc.c | 45 ++++++++++++++++++++++++---------
>>>    target/s390x/vec_int_helper.c   | 16 ------------
>>>    3 files changed, 33 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>> index 56e8149866..ca1e08100a 100644
>>> --- a/target/s390x/helper.h
>>> +++ b/target/s390x/helper.h
>>> @@ -207,8 +207,6 @@ DEF_HELPER_FLAGS_4(gvec_verim16, TCG_CALL_NO_RWG, void,
>>> ptr, cptr, cptr, i32)
>>>    DEF_HELPER_FLAGS_4(gvec_vsl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>>>    DEF_HELPER_FLAGS_4(gvec_vsra, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>>>    DEF_HELPER_FLAGS_4(gvec_vsrl, TCG_CALL_NO_RWG, void, ptr, cptr, i64, i32)
>>> -DEF_HELPER_FLAGS_4(gvec_vscbi8, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>>> -DEF_HELPER_FLAGS_4(gvec_vscbi16, TCG_CALL_NO_RWG, void, ptr, cptr, cptr, i32)
>>>    DEF_HELPER_4(gvec_vtm, void, ptr, cptr, env, i32)
>>>       /* === Vector String Instructions === */
>>> diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
>>> index 5ce7bfb0af..40bcc1604e 100644
>>> --- a/target/s390x/translate_vx.inc.c
>>> +++ b/target/s390x/translate_vx.inc.c
>>> @@ -2130,14 +2130,40 @@ static DisasJumpType op_vs(DisasContext *s, DisasOps
>>> *o)
>>>        return DISAS_NEXT;
>>>    }
>>>    +static void gen_scbi8_i64(TCGv_i64 d, TCGv_i64 a, TCGv_i64 b)
>>> +{
>>> +    TCGv_i64 t = tcg_temp_new_i64();
>>> +
>>> +    tcg_gen_not_i64(t, b);
>>> +    gen_acc(d, a, t, ES_8);
>>> +    tcg_temp_free_i64(t);
>>> +}
>>
>> BTW, I would have thought that we need the 2nd complement in all these
>> cases. However, the description of the other functions confused me
>> (VECTOR SUBTRACT WITH BORROW INDICATION) - add bitwise complement and
>> add the borrow.
>>
>> This passes my test cases (that are verified against real HW), but I am
>> not sure if I check all the corner cases.
>>
>> @Richard, do you have any idea how to do it the right way for this
>> instruction?
>>
> 
> My impression was right. A simple "0-0" test makes this visible. The other two
> fixes seem to be correct, though.

Your description seems to indicate that you want carry output, which is
!borrow.  ARM represents things this way, but I didn't recall it for S390.

If you want to implement sub r,x,y with add r,x,~y, you also have to add one --
often times with the carry-in.  But since we don't have a carry-in here, I
wonder if it isn't easier to invert your result:

     tcg_gen_sub2_i64(tl, th, al, zero, 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);
+    /* "invert" the result: -1 -> 0; 0 -> 1 */
+    tcg_gen_addi_i64(dl, th, 1);


r~

[PATCH v1 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."

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

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 40bcc1604e..87b5790db4 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2207,12 +2207,18 @@ 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 th = tcg_temp_new_i64();
+    TCGv_i64 t = tcg_temp_new_i64();
     TCGv_i64 zero = tcg_const_i64(0);
 
-    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_andi_i64(t, cl, 1);
+    tcg_gen_not_i64(tl, bl);
+    tcg_gen_not_i64(th, bh);
+    tcg_gen_add2_i64(dl, dh, al, ah, tl, th);
+    tcg_gen_add2_i64(dl, dh, dl, dh, t, zero);
     tcg_temp_free_i64(tl);
+    tcg_temp_free_i64(th);
+    tcg_temp_free_i64(t);
     tcg_temp_free_i64(zero);
 }
 
-- 
2.21.0

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

[Richard Henderson]

On 10/18/19 9:10 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."
> 
> Fixes: 48390a7c2716 ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW INDICATION")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/translate_vx.inc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

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


r~

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

[David Hildenbrand]

On 18.10.19 21:11, Richard Henderson wrote:
> On 10/18/19 9:10 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."
>>
>> Fixes: 48390a7c2716 ("s390x/tcg: Implement VECTOR SUBTRACT WITH BORROW INDICATION")
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   target/s390x/translate_vx.inc.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

I'll convert this patch to

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 40bcc1604e..d9403a8163 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2207,13 +2207,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);
 }



-- 

Thanks,

David / dhildenb

[PATCH v1 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."

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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 87b5790db4..2015af9012 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2240,17 +2240,30 @@ 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 sh = tcg_temp_new_i64();
+    TCGv_i64 sl = 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_not_i64(sl, bl);
+    tcg_gen_not_i64(sh, bh);
+
+    /* Add the borrow to the low doubleword of a */
+    tcg_gen_add2_i64(tl, th, al, zero, tl, zero);
+    /* Add the bit-wise complement of b to the low doubleword */
+    tcg_gen_add2_i64(tl, th, tl, th, sl, zero);
+    /* Isolate the carry to the high doubleword */
     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);
+    /* Add the carry to the high doubleword of a */
+    tcg_gen_add2_i64(tl, th, ah, zero, th, zero);
+    /* Add the bit-wise complement of b to the high doubleword */
+    tcg_gen_add2_i64(tl, th, tl, th, sh, zero);
+    /* Isolate the carry to the next doubleword */
     tcg_gen_andi_i64(dl, th, 1);
     tcg_gen_mov_i64(dh, zero);
 
+    tcg_temp_free_i64(sl);
+    tcg_temp_free_i64(sh);
     tcg_temp_free_i64(tl);
     tcg_temp_free_i64(th);
     tcg_temp_free_i64(zero);
-- 
2.21.0

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

[Richard Henderson]

On 10/18/19 9:10 AM, David Hildenbrand wrote:
> +    /* Isolate the carry to the next doubleword */
>      tcg_gen_andi_i64(dl, th, 1);

You can remove this now, since the only possible results are 0/1; it was only
our subtract implementation that produced -1/0.


r~

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

[David Hildenbrand]

On 18.10.19 20:55, Richard Henderson wrote:
> On 10/18/19 9:10 AM, David Hildenbrand wrote:
>> +    /* Isolate the carry to the next doubleword */
>>       tcg_gen_andi_i64(dl, th, 1);
> 
> You can remove this now, since the only possible results are 0/1; it was only
> our subtract implementation that produced -1/0.
> 
> 
> r~
> 

Right, we can simply reuse the VACCC implementation now:

diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
index 87b5790db4..49f9916c37 100644
--- a/target/s390x/translate_vx.inc.c
+++ b/target/s390x/translate_vx.inc.c
@@ -2240,20 +2240,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);
 }

This works as we only have to compute the bitwise complement.

-- 

Thanks,

David / dhildenb