diff mbox series

[v5,33/35] target/arm: Implement SVE dot product (indexed)

Message ID 20180621015359.12018-34-richard.henderson@linaro.org
State New
Headers show
Series target/arm SVE patches | expand

Commit Message

Richard Henderson June 21, 2018, 1:53 a.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.h        |  5 ++
 target/arm/translate-sve.c | 18 +++++++
 target/arm/vec_helper.c    | 96 ++++++++++++++++++++++++++++++++++++++
 target/arm/sve.decode      |  8 +++-
 4 files changed, 126 insertions(+), 1 deletion(-)

Comments

Peter Maydell June 26, 2018, 3:30 p.m. UTC | #1 
On 21 June 2018 at 02:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.h        |  5 ++
>  target/arm/translate-sve.c | 18 +++++++
>  target/arm/vec_helper.c    | 96 ++++++++++++++++++++++++++++++++++++++
>  target/arm/sve.decode      |  8 +++-
>  4 files changed, 126 insertions(+), 1 deletion(-)
>

> +void HELPER(gvec_sdot_idx_b)(void *vd, void *vn, void *vm, uint32_t desc)
> +{
> +    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_4 = opr_sz / 4;
> +    intptr_t index = simd_data(desc);
> +    uint32_t *d = vd;
> +    int8_t *n = vn, *m = vm;
> +
> +    for (i = 0; i < opr_sz_4; i = j) {
> +        int8_t m0 = m[(i + index) * 4 + 0];
> +        int8_t m1 = m[(i + index) * 4 + 1];
> +        int8_t m2 = m[(i + index) * 4 + 2];
> +        int8_t m3 = m[(i + index) * 4 + 3];
> +
> +        j = i;
> +        do {
> +            d[j] += n[j * 4 + 0] * m0
> +                  + n[j * 4 + 1] * m1
> +                  + n[j * 4 + 2] * m2
> +                  + n[j * 4 + 3] * m3;
> +        } while (++j < MIN(i + 4, opr_sz_4));
> +    }
> +    clear_tail(d, opr_sz, simd_maxsz(desc));
> +}

Maybe I'm just half asleep this afternoon, but this is pretty
confusing -- nested loops where the outer loop's increment
uses the inner loop's index, and the inner loop's conditions
depend on the outer loop index...

> diff --git a/target/arm/sve.decode b/target/arm/sve.decode
> index 0b29da9f3a..f69ff285a1 100644
> --- a/target/arm/sve.decode
> +++ b/target/arm/sve.decode
> @@ -722,7 +722,13 @@ UMIN_zzi        00100101 .. 101 011 110 ........ .....          @rdn_i8u
>  MUL_zzi         00100101 .. 110 000 110 ........ .....          @rdn_i8s
>
>  # SVE integer dot product (unpredicated)
> -DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5
> +DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5      ra=%reg_movprfx

Should this have been in the previous patch ?

> +
> +# SVE integer dot product (indexed)
> +DOT_zzx         01000100 101 index:2 rm:3 00000 u:1 rn:5 rd:5 \
> +                sz=0 ra=%reg_movprfx
> +DOT_zzx         01000100 111 index:1 rm:4 00000 u:1 rn:5 rd:5 \
> +                sz=1 ra=%reg_movprfx
>
>  # SVE floating-point complex add (predicated)
>  FCADD           01100100 esz:2 00000 rot:1 100 pg:3 rm:5 rd:5 \
> --
> 2.17.1

thanks
-- PMM
Richard Henderson June 26, 2018, 4:17 p.m. UTC | #2 
On 06/26/2018 08:30 AM, Peter Maydell wrote:
> On 21 June 2018 at 02:53, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/helper.h        |  5 ++
>>  target/arm/translate-sve.c | 18 +++++++
>>  target/arm/vec_helper.c    | 96 ++++++++++++++++++++++++++++++++++++++
>>  target/arm/sve.decode      |  8 +++-
>>  4 files changed, 126 insertions(+), 1 deletion(-)
>>
> 
>> +void HELPER(gvec_sdot_idx_b)(void *vd, void *vn, void *vm, uint32_t desc)
>> +{
>> +    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_4 = opr_sz / 4;
>> +    intptr_t index = simd_data(desc);
>> +    uint32_t *d = vd;
>> +    int8_t *n = vn, *m = vm;
>> +
>> +    for (i = 0; i < opr_sz_4; i = j) {
>> +        int8_t m0 = m[(i + index) * 4 + 0];
>> +        int8_t m1 = m[(i + index) * 4 + 1];
>> +        int8_t m2 = m[(i + index) * 4 + 2];
>> +        int8_t m3 = m[(i + index) * 4 + 3];
>> +
>> +        j = i;
>> +        do {
>> +            d[j] += n[j * 4 + 0] * m0
>> +                  + n[j * 4 + 1] * m1
>> +                  + n[j * 4 + 2] * m2
>> +                  + n[j * 4 + 3] * m3;
>> +        } while (++j < MIN(i + 4, opr_sz_4));
>> +    }
>> +    clear_tail(d, opr_sz, simd_maxsz(desc));
>> +}
> 
> Maybe I'm just half asleep this afternoon, but this is pretty
> confusing -- nested loops where the outer loop's increment
> uses the inner loop's index, and the inner loop's conditions
> depend on the outer loop index...

Yeah, well.

There is an edge case of aa64 advsimd, reusing this same helper,

	sdot	v0.2s, v1.8b, v0.4b[0]

where m values must be read (and held) before writing d results,
and there are not 16/4=4 elements to process but only 2.

I suppose I could special-case oprsz == 8 in order to simplify
iteration of what is otherwise a multiple of 16.

I thought iterating J from I to I+4 was easier to read than
writing out I+J everywhere.  Perhaps not.


>> -DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5
>> +DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5      ra=%reg_movprfx
> 
> Should this have been in the previous patch ?

Yes, thanks.


r~
Peter Maydell June 26, 2018, 4:30 p.m. UTC | #3 
On 26 June 2018 at 17:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 06/26/2018 08:30 AM, Peter Maydell wrote:
>> On 21 June 2018 at 02:53, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  target/arm/helper.h        |  5 ++
>>>  target/arm/translate-sve.c | 18 +++++++
>>>  target/arm/vec_helper.c    | 96 ++++++++++++++++++++++++++++++++++++++
>>>  target/arm/sve.decode      |  8 +++-
>>>  4 files changed, 126 insertions(+), 1 deletion(-)
>>>
>>
>>> +void HELPER(gvec_sdot_idx_b)(void *vd, void *vn, void *vm, uint32_t desc)
>>> +{
>>> +    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_4 = opr_sz / 4;
>>> +    intptr_t index = simd_data(desc);
>>> +    uint32_t *d = vd;
>>> +    int8_t *n = vn, *m = vm;
>>> +
>>> +    for (i = 0; i < opr_sz_4; i = j) {
>>> +        int8_t m0 = m[(i + index) * 4 + 0];
>>> +        int8_t m1 = m[(i + index) * 4 + 1];
>>> +        int8_t m2 = m[(i + index) * 4 + 2];
>>> +        int8_t m3 = m[(i + index) * 4 + 3];
>>> +
>>> +        j = i;
>>> +        do {
>>> +            d[j] += n[j * 4 + 0] * m0
>>> +                  + n[j * 4 + 1] * m1
>>> +                  + n[j * 4 + 2] * m2
>>> +                  + n[j * 4 + 3] * m3;
>>> +        } while (++j < MIN(i + 4, opr_sz_4));
>>> +    }
>>> +    clear_tail(d, opr_sz, simd_maxsz(desc));
>>> +}
>>
>> Maybe I'm just half asleep this afternoon, but this is pretty
>> confusing -- nested loops where the outer loop's increment
>> uses the inner loop's index, and the inner loop's conditions
>> depend on the outer loop index...
>
> Yeah, well.
>
> There is an edge case of aa64 advsimd, reusing this same helper,
>
>         sdot    v0.2s, v1.8b, v0.4b[0]
>
> where m values must be read (and held) before writing d results,
> and there are not 16/4=4 elements to process but only 2.
>
> I suppose I could special-case oprsz == 8 in order to simplify
> iteration of what is otherwise a multiple of 16.
>
> I thought iterating J from I to I+4 was easier to read than
> writing out I+J everywhere.  Perhaps not.

Mmm. I did indeed fail to notice the symmetry between the
indexes into m[] and those into n[].
The other bit that threw me is where the outer loop on i
updates using j.

A comment describing the intent might assist ?

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.h b/target/arm/helper.h
index e23ce7ff19..59e8c3bd1b 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -588,6 +588,11 @@  DEF_HELPER_FLAGS_4(gvec_udot_b, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_4(gvec_sdot_h, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_4(gvec_udot_h, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
 
+DEF_HELPER_FLAGS_4(gvec_sdot_idx_b, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
+DEF_HELPER_FLAGS_4(gvec_udot_idx_b, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
+DEF_HELPER_FLAGS_4(gvec_sdot_idx_h, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
+DEF_HELPER_FLAGS_4(gvec_udot_idx_h, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
+
 DEF_HELPER_FLAGS_5(gvec_fcaddh, TCG_CALL_NO_RWG,
                    void, ptr, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_5(gvec_fcadds, TCG_CALL_NO_RWG,
diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c
index aa109208e5..af2958be10 100644
--- a/target/arm/translate-sve.c
+++ b/target/arm/translate-sve.c
@@ -3440,6 +3440,24 @@  static bool trans_DOT_zzz(DisasContext *s, arg_DOT_zzz *a, uint32_t insn)
     return true;
 }
 
+static bool trans_DOT_zzx(DisasContext *s, arg_DOT_zzx *a, uint32_t insn)
+{
+    static gen_helper_gvec_3 * const fns[2][2] = {
+        { gen_helper_gvec_sdot_idx_b, gen_helper_gvec_sdot_idx_h },
+        { gen_helper_gvec_udot_idx_b, gen_helper_gvec_udot_idx_h }
+    };
+
+    if (sve_access_check(s)) {
+        unsigned vsz = vec_full_reg_size(s);
+        tcg_gen_gvec_3_ool(vec_full_reg_offset(s, a->rd),
+                           vec_full_reg_offset(s, a->rn),
+                           vec_full_reg_offset(s, a->rm),
+                           vsz, vsz, a->index, fns[a->u][a->sz]);
+    }
+    return true;
+}
+
+
 /*
  *** SVE Floating Point Multiply-Add Indexed Group
  */
diff --git a/target/arm/vec_helper.c b/target/arm/vec_helper.c
index c16a30c3b5..3117ee39cd 100644
--- a/target/arm/vec_helper.c
+++ b/target/arm/vec_helper.c
@@ -261,6 +261,102 @@  void HELPER(gvec_udot_h)(void *vd, void *vn, void *vm, uint32_t desc)
     clear_tail(d, opr_sz, simd_maxsz(desc));
 }
 
+void HELPER(gvec_sdot_idx_b)(void *vd, void *vn, void *vm, uint32_t desc)
+{
+    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_4 = opr_sz / 4;
+    intptr_t index = simd_data(desc);
+    uint32_t *d = vd;
+    int8_t *n = vn, *m = vm;
+
+    for (i = 0; i < opr_sz_4; i = j) {
+        int8_t m0 = m[(i + index) * 4 + 0];
+        int8_t m1 = m[(i + index) * 4 + 1];
+        int8_t m2 = m[(i + index) * 4 + 2];
+        int8_t m3 = m[(i + index) * 4 + 3];
+
+        j = i;
+        do {
+            d[j] += n[j * 4 + 0] * m0
+                  + n[j * 4 + 1] * m1
+                  + n[j * 4 + 2] * m2
+                  + n[j * 4 + 3] * m3;
+        } while (++j < MIN(i + 4, opr_sz_4));
+    }
+    clear_tail(d, opr_sz, simd_maxsz(desc));
+}
+
+void HELPER(gvec_udot_idx_b)(void *vd, void *vn, void *vm, uint32_t desc)
+{
+    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_4 = opr_sz / 4;
+    intptr_t index = simd_data(desc);
+    uint32_t *d = vd;
+    uint8_t *n = vn, *m = vm;
+
+    for (i = 0; i < opr_sz_4; i = j) {
+        uint8_t m0 = m[(i + index) * 4 + 0];
+        uint8_t m1 = m[(i + index) * 4 + 1];
+        uint8_t m2 = m[(i + index) * 4 + 2];
+        uint8_t m3 = m[(i + index) * 4 + 3];
+
+        j = i;
+        do {
+            d[j] += n[j * 4 + 0] * m0
+                  + n[j * 4 + 1] * m1
+                  + n[j * 4 + 2] * m2
+                  + n[j * 4 + 3] * m3;
+        } while (++j < MIN(i + 4, opr_sz_4));
+    }
+    clear_tail(d, opr_sz, simd_maxsz(desc));
+}
+
+void HELPER(gvec_sdot_idx_h)(void *vd, void *vn, void *vm, uint32_t desc)
+{
+    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_8 = opr_sz / 8;
+    intptr_t index = simd_data(desc);
+    uint64_t *d = vd;
+    int16_t *n = vn, *m = vm;
+
+    for (i = 0; i < opr_sz_8; i = j) {
+        int64_t m0 = m[(i + index) * 4 + 0];
+        int64_t m1 = m[(i + index) * 4 + 1];
+        int64_t m2 = m[(i + index) * 4 + 2];
+        int64_t m3 = m[(i + index) * 4 + 3];
+
+        j = i;
+        do {
+            d[j] += n[j * 4 + 0] * m0
+                  + n[j * 4 + 1] * m1
+                  + n[j * 4 + 2] * m2
+                  + n[j * 4 + 3] * m3;
+        } while (++j < MIN(i + 2, opr_sz_8));
+    }
+    clear_tail(d, opr_sz, simd_maxsz(desc));
+}
+
+void HELPER(gvec_udot_idx_h)(void *vd, void *vn, void *vm, uint32_t desc)
+{
+    intptr_t i, j, opr_sz = simd_oprsz(desc), opr_sz_8 = opr_sz / 8;
+    intptr_t index = simd_data(desc);
+    uint64_t *d = vd;
+    uint16_t *n = vn, *m = vm;
+
+    for (i = 0; i < opr_sz_8; i = j) {
+        uint64_t m0 = m[(i + index) * 4 + 0];
+        uint64_t m1 = m[(i + index) * 4 + 1];
+        uint64_t m2 = m[(i + index) * 4 + 2];
+        uint64_t m3 = m[(i + index) * 4 + 3];
+
+        j = i;
+        do {
+            d[j] += n[j * 4 + 0] * m0
+                  + n[j * 4 + 1] * m1
+                  + n[j * 4 + 2] * m2
+                  + n[j * 4 + 3] * m3;
+        } while (++j < MIN(i + 2, opr_sz_8));
+    }
+    clear_tail(d, opr_sz, simd_maxsz(desc));
+}
+
 void HELPER(gvec_fcaddh)(void *vd, void *vn, void *vm,
                          void *vfpst, uint32_t desc)
 {
diff --git a/target/arm/sve.decode b/target/arm/sve.decode
index 0b29da9f3a..f69ff285a1 100644
--- a/target/arm/sve.decode
+++ b/target/arm/sve.decode
@@ -722,7 +722,13 @@  UMIN_zzi        00100101 .. 101 011 110 ........ .....          @rdn_i8u
 MUL_zzi         00100101 .. 110 000 110 ........ .....          @rdn_i8s
 
 # SVE integer dot product (unpredicated)
-DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5
+DOT_zzz         01000100 1 sz:1 0 rm:5 00000 u:1 rn:5 rd:5      ra=%reg_movprfx
+
+# SVE integer dot product (indexed)
+DOT_zzx         01000100 101 index:2 rm:3 00000 u:1 rn:5 rd:5 \
+                sz=0 ra=%reg_movprfx
+DOT_zzx         01000100 111 index:1 rm:4 00000 u:1 rn:5 rd:5 \
+                sz=1 ra=%reg_movprfx
 
 # SVE floating-point complex add (predicated)
 FCADD           01100100 esz:2 00000 rot:1 100 pg:3 rm:5 rd:5 \