| Message ID | 20210607165821.9892-14-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: First slice of MVE implementation | expand |
On 6/7/21 9:57 AM, Peter Maydell wrote: > Implement the MVE VCLZ insn (and the necessary machinery > for MVE 1-input vector ops). > > Note that for non-load instructions predication is always performed > at a byte level granularity regardless of element size (R_ZLSJ), > and so the masking logic here differs from that used in the VLDR > and VSTR helpers. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/helper-mve.h | 4 ++++ > target/arm/mve.decode | 8 +++++++ > target/arm/mve_helper.c | 48 ++++++++++++++++++++++++++++++++++++++ > target/arm/translate-mve.c | 43 ++++++++++++++++++++++++++++++++++ > 4 files changed, 103 insertions(+) > > diff --git a/target/arm/helper-mve.h b/target/arm/helper-mve.h > index e47d4164ae7..c5c1315b161 100644 > --- a/target/arm/helper-mve.h > +++ b/target/arm/helper-mve.h > @@ -32,3 +32,7 @@ DEF_HELPER_FLAGS_3(mve_vldrh_uw, TCG_CALL_NO_WG, void, env, ptr, i32) > DEF_HELPER_FLAGS_3(mve_vstrb_h, TCG_CALL_NO_WG, void, env, ptr, i32) > DEF_HELPER_FLAGS_3(mve_vstrb_w, TCG_CALL_NO_WG, void, env, ptr, i32) > DEF_HELPER_FLAGS_3(mve_vstrh_w, TCG_CALL_NO_WG, void, env, ptr, i32) > + > +DEF_HELPER_FLAGS_3(mve_vclzb, TCG_CALL_NO_WG, void, env, ptr, ptr) > +DEF_HELPER_FLAGS_3(mve_vclzh, TCG_CALL_NO_WG, void, env, ptr, ptr) > +DEF_HELPER_FLAGS_3(mve_vclzw, TCG_CALL_NO_WG, void, env, ptr, ptr) > diff --git a/target/arm/mve.decode b/target/arm/mve.decode > index 3bc5f034531..24999bf703e 100644 > --- a/target/arm/mve.decode > +++ b/target/arm/mve.decode > @@ -20,13 +20,17 @@ > # > > %qd 22:1 13:3 > +%qm 5:1 1:3 > > &vldr_vstr rn qd imm p a w size l u > +&1op qd qm size > > @vldr_vstr ....... . . . . l:1 rn:4 ... ...... imm:7 &vldr_vstr qd=%qd u=0 > # Note that both Rn and Qd are 3 bits only (no D bit) > @vldst_wn ... u:1 ... . . . . l:1 . rn:3 qd:3 . ... .. imm:7 &vldr_vstr > > +@1op .... .... .... size:2 .. .... .... .... .... &1op qd=%qd qm=%qm > + > # Vector loads and stores > > # Widening loads and narrowing stores: > @@ -61,3 +65,7 @@ VLDR_VSTR 1110110 1 a:1 . w:1 . .... ... 111101 ....... @vldr_vstr \ > size=1 p=1 > VLDR_VSTR 1110110 1 a:1 . w:1 . .... ... 111110 ....... @vldr_vstr \ > size=2 p=1 > + > +# Vector miscellaneous > + > +VCLZ 1111 1111 1 . 11 .. 00 ... 0 0100 11 . 0 ... 0 @1op > diff --git a/target/arm/mve_helper.c b/target/arm/mve_helper.c > index 6a2fc1c37cd..b7c44f57c09 100644 > --- a/target/arm/mve_helper.c > +++ b/target/arm/mve_helper.c > @@ -196,3 +196,51 @@ DO_VSTR(vstrh_w, 4, stw, int32_t, H4) > > #undef DO_VLDR > #undef DO_VSTR > + > +/* > + * Take the bottom bits of mask (which is 1 bit per lane) and > + * convert to a mask which has 1s in each byte which is predicated. > + */ > +static uint8_t mask_to_bytemask1(uint16_t mask) > +{ > + return (mask & 1) ? 0xff : 0; > +} > + > +static uint16_t mask_to_bytemask2(uint16_t mask) > +{ > + static const uint16_t masks[] = { 0x0000, 0x00ff, 0xff00, 0xffff }; > + return masks[mask & 3]; > +} > + > +static uint32_t mask_to_bytemask4(uint16_t mask) > +{ > + static const uint32_t masks[] = { > + 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, > + 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, > + 0xff000000, 0xff0000ff, 0xff00ff00, 0xff00ffff, > + 0xffff0000, 0xffff00ff, 0xffffff00, 0xffffffff, > + }; I'll note that (1) the values for the mask_to_bytemask2 array overlap the first 4 values of the mask_to_bytemask4 array, and (2) both of these overlap with the larger static inline uint64_t expand_pred_b(uint8_t byte) from SVE. It'd be nice to share the storage, whatever the actual functional interface into the array. > +#define DO_1OP(OP, ESIZE, TYPE, H, FN) \ > + void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm) \ > + { \ > + TYPE *d = vd, *m = vm; \ > + uint16_t mask = mve_element_mask(env); \ > + unsigned e; \ > + for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ > + TYPE r = FN(m[H(e)]); \ > + uint64_t bytemask = mask_to_bytemask##ESIZE(mask); \ Why uint64_t and not TYPE? Or uint32_t? > + if (!mve_eci_check(s)) { > + return true; > + } > + > + if (!vfp_access_check(s)) { > + return true; > + } Not the first instance, but is it worth saving 4 lines per and combining these into one IF? > +#define DO_1OP(INSN, FN) \ > + static bool trans_##INSN(DisasContext *s, arg_1op *a) \ > + { \ > + MVEGenOneOpFn *fns[] = { \ static const. r~
On Tue, 8 Jun 2021 at 23:10, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 6/7/21 9:57 AM, Peter Maydell wrote: > > Implement the MVE VCLZ insn (and the necessary machinery > > for MVE 1-input vector ops). > > > > Note that for non-load instructions predication is always performed > > at a byte level granularity regardless of element size (R_ZLSJ), > > and so the masking logic here differs from that used in the VLDR > > and VSTR helpers. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > + > > +/* > > + * Take the bottom bits of mask (which is 1 bit per lane) and > > + * convert to a mask which has 1s in each byte which is predicated. > > + */ > > +static uint8_t mask_to_bytemask1(uint16_t mask) > > +{ > > + return (mask & 1) ? 0xff : 0; > > +} > > + > > +static uint16_t mask_to_bytemask2(uint16_t mask) > > +{ > > + static const uint16_t masks[] = { 0x0000, 0x00ff, 0xff00, 0xffff }; > > + return masks[mask & 3]; > > +} > > + > > +static uint32_t mask_to_bytemask4(uint16_t mask) > > +{ > > + static const uint32_t masks[] = { > > + 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, > > + 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, > > + 0xff000000, 0xff0000ff, 0xff00ff00, 0xff00ffff, > > + 0xffff0000, 0xffff00ff, 0xffffff00, 0xffffffff, > > + }; > > I'll note that > > (1) the values for the mask_to_bytemask2 array overlap the first 4 values of > the mask_to_bytemask4 array, and > > (2) both of these overlap with the larger > > static inline uint64_t expand_pred_b(uint8_t byte) > > from SVE. It'd be nice to share the storage, whatever the actual functional > interface into the array. Yeah, I guess so. I didn't really feel like trying to abstract that out... > > +#define DO_1OP(OP, ESIZE, TYPE, H, FN) \ > > + void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm) \ > > + { \ > > + TYPE *d = vd, *m = vm; \ > > + uint16_t mask = mve_element_mask(env); \ > > + unsigned e; \ > > + for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ > > + TYPE r = FN(m[H(e)]); \ > > + uint64_t bytemask = mask_to_bytemask##ESIZE(mask); \ > > Why uint64_t and not TYPE? Or uint32_t? A later patch adds the mask_to_bytemask8(), so I wanted a type that was definitely unsigned (so TYPE isn't any good) and which was definitely big enough for 64 bits. > > + if (!mve_eci_check(s)) { > > + return true; > > + } > > + > > + if (!vfp_access_check(s)) { > > + return true; > > + } > > Not the first instance, but is it worth saving 4 lines per and combining these > into one IF? Yes, I think so. -- PMM
On 6/10/21 5:40 AM, Peter Maydell wrote: >>> +#define DO_1OP(OP, ESIZE, TYPE, H, FN) \ >>> + void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm) \ >>> + { \ >>> + TYPE *d = vd, *m = vm; \ >>> + uint16_t mask = mve_element_mask(env); \ >>> + unsigned e; \ >>> + for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ >>> + TYPE r = FN(m[H(e)]); \ >>> + uint64_t bytemask = mask_to_bytemask##ESIZE(mask); \ >> >> Why uint64_t and not TYPE? Or uint32_t? > > A later patch adds the mask_to_bytemask8(), so I wanted > a type that was definitely unsigned (so TYPE isn't any good) > and which was definitely big enough for 64 bits. Hmm. I was just concerned about the unnecessary type extension involved. What about changing the interface. Not to return a mask as you do here, but to perform the entire merge operation. E.g. static uint8_t mergemask1(uint8_t d, uint8_t r, uint16_t mask) { return mask & 1 ? r : d; } static uint16_t mergemask2(uint16_t d, uint16_t r, uint16_t mask) { uint16_t bmask = array_whotsit[mask & 3]; return (d & ~bmask) | (r & bmask); } etc. Or maybe with a pointer argument for D, so that the load+store is done there as well. In which case you could use QEMU_GENERIC to select the function invoked, instead of using token pasting everywhere. E.g. static void mergemask_ub(uint8_t *d, uint8_r, uint16_t mask) { if (mask & 1) { *d = r; } } static void mergemask_sb(int8_t *d, int8_r, uint16_t mask) { mergemask_ub((uint8_t *)d, r, mask); } static void mergemask_uh(uint16_t *d, uint16_r, uint16_t mask) { uint16_t bmask = array_whotsit[mask & 3]; *d = (*d & ~bmask) | (r & bmask); } ... #define mergemask(D, R, M) \ QEMU_GENERIC(D, (uint8_t *, mergemask_ub), \ (int8_t *, mergemask_sb), \ ... ) BTW, now that we're at minimal gcc 7, I think we can shift to -std=gnu11 so that we can drop QEMU_GENERIC and just use _Generic, which is much easier to read than the above, and will give better error messages for missing cases. Anyway... Which takes your boilerplate down to > +#define DO_1OP(OP, ESIZE, TYPE, H, FN) \ > + void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm) \ > + { \ > + TYPE *d = vd, *m = vm; \ > + uint16_t mask = mve_element_mask(env); \ > + for (unsigned e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ > + mergemask(&d[H(e)], FN(m[H(e)]), mask); \ > + } \ > + mve_advance_vpt(env); \ > + } which looks pretty tidy to me. r~
diff --git a/target/arm/helper-mve.h b/target/arm/helper-mve.h index e47d4164ae7..c5c1315b161 100644 --- a/target/arm/helper-mve.h +++ b/target/arm/helper-mve.h @@ -32,3 +32,7 @@ DEF_HELPER_FLAGS_3(mve_vldrh_uw, TCG_CALL_NO_WG, void, env, ptr, i32) DEF_HELPER_FLAGS_3(mve_vstrb_h, TCG_CALL_NO_WG, void, env, ptr, i32) DEF_HELPER_FLAGS_3(mve_vstrb_w, TCG_CALL_NO_WG, void, env, ptr, i32) DEF_HELPER_FLAGS_3(mve_vstrh_w, TCG_CALL_NO_WG, void, env, ptr, i32) + +DEF_HELPER_FLAGS_3(mve_vclzb, TCG_CALL_NO_WG, void, env, ptr, ptr) +DEF_HELPER_FLAGS_3(mve_vclzh, TCG_CALL_NO_WG, void, env, ptr, ptr) +DEF_HELPER_FLAGS_3(mve_vclzw, TCG_CALL_NO_WG, void, env, ptr, ptr) diff --git a/target/arm/mve.decode b/target/arm/mve.decode index 3bc5f034531..24999bf703e 100644 --- a/target/arm/mve.decode +++ b/target/arm/mve.decode @@ -20,13 +20,17 @@ # %qd 22:1 13:3 +%qm 5:1 1:3 &vldr_vstr rn qd imm p a w size l u +&1op qd qm size @vldr_vstr ....... . . . . l:1 rn:4 ... ...... imm:7 &vldr_vstr qd=%qd u=0 # Note that both Rn and Qd are 3 bits only (no D bit) @vldst_wn ... u:1 ... . . . . l:1 . rn:3 qd:3 . ... .. imm:7 &vldr_vstr +@1op .... .... .... size:2 .. .... .... .... .... &1op qd=%qd qm=%qm + # Vector loads and stores # Widening loads and narrowing stores: @@ -61,3 +65,7 @@ VLDR_VSTR 1110110 1 a:1 . w:1 . .... ... 111101 ....... @vldr_vstr \ size=1 p=1 VLDR_VSTR 1110110 1 a:1 . w:1 . .... ... 111110 ....... @vldr_vstr \ size=2 p=1 + +# Vector miscellaneous + +VCLZ 1111 1111 1 . 11 .. 00 ... 0 0100 11 . 0 ... 0 @1op diff --git a/target/arm/mve_helper.c b/target/arm/mve_helper.c index 6a2fc1c37cd..b7c44f57c09 100644 --- a/target/arm/mve_helper.c +++ b/target/arm/mve_helper.c @@ -196,3 +196,51 @@ DO_VSTR(vstrh_w, 4, stw, int32_t, H4) #undef DO_VLDR #undef DO_VSTR + +/* + * Take the bottom bits of mask (which is 1 bit per lane) and + * convert to a mask which has 1s in each byte which is predicated. + */ +static uint8_t mask_to_bytemask1(uint16_t mask) +{ + return (mask & 1) ? 0xff : 0; +} + +static uint16_t mask_to_bytemask2(uint16_t mask) +{ + static const uint16_t masks[] = { 0x0000, 0x00ff, 0xff00, 0xffff }; + return masks[mask & 3]; +} + +static uint32_t mask_to_bytemask4(uint16_t mask) +{ + static const uint32_t masks[] = { + 0x00000000, 0x000000ff, 0x0000ff00, 0x0000ffff, + 0x00ff0000, 0x00ff00ff, 0x00ffff00, 0x00ffffff, + 0xff000000, 0xff0000ff, 0xff00ff00, 0xff00ffff, + 0xffff0000, 0xffff00ff, 0xffffff00, 0xffffffff, + }; + return masks[mask & 0xf]; +} + +#define DO_1OP(OP, ESIZE, TYPE, H, FN) \ + void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm) \ + { \ + TYPE *d = vd, *m = vm; \ + uint16_t mask = mve_element_mask(env); \ + unsigned e; \ + for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ + TYPE r = FN(m[H(e)]); \ + uint64_t bytemask = mask_to_bytemask##ESIZE(mask); \ + d[H(e)] &= ~bytemask; \ + d[H(e)] |= (r & bytemask); \ + } \ + mve_advance_vpt(env); \ + } + +#define DO_CLZ_B(N) (clz32(N) - 24) +#define DO_CLZ_H(N) (clz32(N) - 16) + +DO_1OP(vclzb, 1, uint8_t, H1, DO_CLZ_B) +DO_1OP(vclzh, 2, uint16_t, H2, DO_CLZ_H) +DO_1OP(vclzw, 4, uint32_t, H4, clz32) diff --git a/target/arm/translate-mve.c b/target/arm/translate-mve.c index 14206893d5f..6bbc2df35c1 100644 --- a/target/arm/translate-mve.c +++ b/target/arm/translate-mve.c @@ -29,6 +29,7 @@ #include "decode-mve.c.inc" typedef void MVEGenLdStFn(TCGv_ptr, TCGv_ptr, TCGv_i32); +typedef void MVEGenOneOpFn(TCGv_ptr, TCGv_ptr, TCGv_ptr); /* Return the offset of a Qn register (same semantics as aa32_vfp_qreg()) */ static inline long mve_qreg_offset(unsigned reg) @@ -167,3 +168,45 @@ static bool trans_VLDR_VSTR(DisasContext *s, arg_VLDR_VSTR *a) DO_VLDST_WIDE_NARROW(VLDSTB_H, vldrb_sh, vldrb_uh, vstrb_h) DO_VLDST_WIDE_NARROW(VLDSTB_W, vldrb_sw, vldrb_uw, vstrb_w) DO_VLDST_WIDE_NARROW(VLDSTH_W, vldrh_sw, vldrh_uw, vstrh_w) + +static bool do_1op(DisasContext *s, arg_1op *a, MVEGenOneOpFn fn) +{ + TCGv_ptr qd, qm; + + if (!dc_isar_feature(aa32_mve, s)) { + return false; + } + if (a->qd > 7 || a->qm > 7 || !fn) { + return false; + } + + if (!mve_eci_check(s)) { + return true; + } + + if (!vfp_access_check(s)) { + return true; + } + + qd = mve_qreg_ptr(a->qd); + qm = mve_qreg_ptr(a->qm); + fn(cpu_env, qd, qm); + tcg_temp_free_ptr(qd); + tcg_temp_free_ptr(qm); + mve_update_eci(s); + return true; +} + +#define DO_1OP(INSN, FN) \ + static bool trans_##INSN(DisasContext *s, arg_1op *a) \ + { \ + MVEGenOneOpFn *fns[] = { \ + gen_helper_mve_##FN##b, \ + gen_helper_mve_##FN##h, \ + gen_helper_mve_##FN##w, \ + NULL, \ + }; \ + return do_1op(s, a, fns[a->size]); \ + } + +DO_1OP(VCLZ, vclz)
Implement the MVE VCLZ insn (and the necessary machinery for MVE 1-input vector ops). Note that for non-load instructions predication is always performed at a byte level granularity regardless of element size (R_ZLSJ), and so the masking logic here differs from that used in the VLDR and VSTR helpers. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/helper-mve.h | 4 ++++ target/arm/mve.decode | 8 +++++++ target/arm/mve_helper.c | 48 ++++++++++++++++++++++++++++++++++++++ target/arm/translate-mve.c | 43 ++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+) -- 2.20.1