Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Thanks for the reviews! I decided to take this patch (acks included) and send it in together with a cleanup of the env_archcpu() usages in vector_helper.c: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg07566.html Thanks, Daniel On 2/21/23 15:45, Daniel Henrique Barboza wrote: Commit 752614cab8e6 ("target/riscv: rvv: Add tail agnostic for vector load / store instructions") added code to set the tail elements to 1 in the end of vext_ldst_stride(), vext_ldst_us(), vext_ldst_index() and vext_ldff(). Aside from a env->vl versus an evl value being used in the first loop, the code is being repeated 4 times. Create a helper to avoid code repetition in all those functions. Arguments that are used in the callers (nf, esz and max_elems) are passed as arguments. All other values are being derived inside the helper. Signed-off-by: Daniel Henrique Barboza --- target/riscv/vector_helper.c | 86 +--- 1 file changed, 30 insertions(+), 56 deletions(-) diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c index 00de879787..7d2e3978f1 100644 --- a/target/riscv/vector_helper.c +++ b/target/riscv/vector_helper.c @@ -267,6 +267,28 @@ GEN_VEXT_ST_ELEM(ste_h, int16_t, H2, stw) GEN_VEXT_ST_ELEM(ste_w, int32_t, H4, stl) GEN_VEXT_ST_ELEM(ste_d, int64_t, H8, stq) +static void vext_set_tail_elems_1s(CPURISCVState *env, target_ulong vl, + void *vd, uint32_t desc, uint32_t nf, + uint32_t esz, uint32_t max_elems) +{ +uint32_t total_elems = vext_get_total_elems(env, desc, esz); +uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; +uint32_t vta = vext_vta(desc); +uint32_t registers_used; +int k; + +for (k = 0; k < nf; ++k) { +vext_set_elems_1s(vd, vta, (k * max_elems + vl) * esz, + (k * max_elems + max_elems) * esz); +} + +if (nf * max_elems % total_elems != 0) { +registers_used = ((nf * max_elems) * esz + (vlenb - 1)) / vlenb; +vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, + registers_used * vlenb); +} +} + /* *** stride: access vector element from strided memory */ @@ -281,8 +303,6 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc); for (i = env->vstart; i < env->vl; i++, env->vstart++) { @@ -301,18 +321,8 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, env->vl, vd, desc, nf, esz, max_elems); } #define GEN_VEXT_LD_STRIDE(NAME, ETYPE, LOAD_FN)\ @@ -359,8 +369,6 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); /* load bytes from guest memory */ for (i = env->vstart; i < evl; i++, env->vstart++) { @@ -372,18 +380,8 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + evl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, evl, vd, desc, nf, esz, max_elems); } /* @@ -484,8 +482,6 @@ vext_ldst_index(void *vd, void *v0, target_ulong base, uint32_t vm = vext_vm(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc);
Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Reviewed-by: Frank Chang On Wed, Feb 22, 2023 at 2:46 AM Daniel Henrique Barboza < dbarb...@ventanamicro.com> wrote: > Commit 752614cab8e6 ("target/riscv: rvv: Add tail agnostic for vector > load / store instructions") added code to set the tail elements to 1 in > the end of vext_ldst_stride(), vext_ldst_us(), vext_ldst_index() and > vext_ldff(). Aside from a env->vl versus an evl value being used in the > first loop, the code is being repeated 4 times. > > Create a helper to avoid code repetition in all those functions. > Arguments that are used in the callers (nf, esz and max_elems) are > passed as arguments. All other values are being derived inside the > helper. > > Signed-off-by: Daniel Henrique Barboza > --- > target/riscv/vector_helper.c | 86 +--- > 1 file changed, 30 insertions(+), 56 deletions(-) > > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index 00de879787..7d2e3978f1 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -267,6 +267,28 @@ GEN_VEXT_ST_ELEM(ste_h, int16_t, H2, stw) > GEN_VEXT_ST_ELEM(ste_w, int32_t, H4, stl) > GEN_VEXT_ST_ELEM(ste_d, int64_t, H8, stq) > > +static void vext_set_tail_elems_1s(CPURISCVState *env, target_ulong vl, > + void *vd, uint32_t desc, uint32_t nf, > + uint32_t esz, uint32_t max_elems) > +{ > +uint32_t total_elems = vext_get_total_elems(env, desc, esz); > +uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; > +uint32_t vta = vext_vta(desc); > +uint32_t registers_used; > +int k; > + > +for (k = 0; k < nf; ++k) { > +vext_set_elems_1s(vd, vta, (k * max_elems + vl) * esz, > + (k * max_elems + max_elems) * esz); > +} > + > +if (nf * max_elems % total_elems != 0) { > +registers_used = ((nf * max_elems) * esz + (vlenb - 1)) / vlenb; > +vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, > + registers_used * vlenb); > +} > +} > + > /* > *** stride: access vector element from strided memory > */ > @@ -281,8 +303,6 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, > uint32_t nf = vext_nf(desc); > uint32_t max_elems = vext_max_elems(desc, log2_esz); > uint32_t esz = 1 << log2_esz; > -uint32_t total_elems = vext_get_total_elems(env, desc, esz); > -uint32_t vta = vext_vta(desc); > uint32_t vma = vext_vma(desc); > > for (i = env->vstart; i < env->vl; i++, env->vstart++) { > @@ -301,18 +321,8 @@ vext_ldst_stride(void *vd, void *v0, target_ulong > base, > } > } > env->vstart = 0; > -/* set tail elements to 1s */ > -for (k = 0; k < nf; ++k) { > -vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz, > - (k * max_elems + max_elems) * esz); > -} > -if (nf * max_elems % total_elems != 0) { > -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; > -uint32_t registers_used = > -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; > -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, > - registers_used * vlenb); > -} > + > +vext_set_tail_elems_1s(env, env->vl, vd, desc, nf, esz, max_elems); > } > > #define GEN_VEXT_LD_STRIDE(NAME, ETYPE, LOAD_FN)\ > @@ -359,8 +369,6 @@ vext_ldst_us(void *vd, target_ulong base, > CPURISCVState *env, uint32_t desc, > uint32_t nf = vext_nf(desc); > uint32_t max_elems = vext_max_elems(desc, log2_esz); > uint32_t esz = 1 << log2_esz; > -uint32_t total_elems = vext_get_total_elems(env, desc, esz); > -uint32_t vta = vext_vta(desc); > > /* load bytes from guest memory */ > for (i = env->vstart; i < evl; i++, env->vstart++) { > @@ -372,18 +380,8 @@ vext_ldst_us(void *vd, target_ulong base, > CPURISCVState *env, uint32_t desc, > } > } > env->vstart = 0; > -/* set tail elements to 1s */ > -for (k = 0; k < nf; ++k) { > -vext_set_elems_1s(vd, vta, (k * max_elems + evl) * esz, > - (k * max_elems + max_elems) * esz); > -} > -if (nf * max_elems % total_elems != 0) { > -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; > -uint32_t registers_used = > -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; > -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, > - registers_used * vlenb); > -} > + > +vext_set_tail_elems_1s(env, evl, vd, desc, nf, esz, max_elems); > } > > /* > @@ -484,8 +482,6 @@ vext_ldst_index(void *vd, void *v0, target_ulong base, > uint32_t vm = vext_vm(desc); > uint32_t max_elems = vext_max_elems(desc, log2_esz); > uint32_t esz = 1 << log2_esz; > -uint32_t total_elems = vext_get_total_elems(env, desc, esz); > -uint32_t vta = vext_vta(desc); > uint32_t vma = vext_vma(desc); > >
Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
On 2023/2/22 02:45, Daniel Henrique Barboza wrote: Commit 752614cab8e6 ("target/riscv: rvv: Add tail agnostic for vector load / store instructions") added code to set the tail elements to 1 in the end of vext_ldst_stride(), vext_ldst_us(), vext_ldst_index() and vext_ldff(). Aside from a env->vl versus an evl value being used in the first loop, the code is being repeated 4 times. Create a helper to avoid code repetition in all those functions. Arguments that are used in the callers (nf, esz and max_elems) are passed as arguments. All other values are being derived inside the helper. Signed-off-by: Daniel Henrique Barboza LGTM. Reviewed-by: Weiwei Li --- target/riscv/vector_helper.c | 86 +--- 1 file changed, 30 insertions(+), 56 deletions(-) diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c index 00de879787..7d2e3978f1 100644 --- a/target/riscv/vector_helper.c +++ b/target/riscv/vector_helper.c @@ -267,6 +267,28 @@ GEN_VEXT_ST_ELEM(ste_h, int16_t, H2, stw) GEN_VEXT_ST_ELEM(ste_w, int32_t, H4, stl) GEN_VEXT_ST_ELEM(ste_d, int64_t, H8, stq) +static void vext_set_tail_elems_1s(CPURISCVState *env, target_ulong vl, + void *vd, uint32_t desc, uint32_t nf, + uint32_t esz, uint32_t max_elems) +{ +uint32_t total_elems = vext_get_total_elems(env, desc, esz); +uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; By the way, env_archcpu(env)->cfg in vector_helper.c can also be replace by cpu_get_cfg(). Regards, Weiwei Li +uint32_t vta = vext_vta(desc); +uint32_t registers_used; +int k; + +for (k = 0; k < nf; ++k) { +vext_set_elems_1s(vd, vta, (k * max_elems + vl) * esz, + (k * max_elems + max_elems) * esz); +} + +if (nf * max_elems % total_elems != 0) { +registers_used = ((nf * max_elems) * esz + (vlenb - 1)) / vlenb; +vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, + registers_used * vlenb); +} +} + /* *** stride: access vector element from strided memory */ @@ -281,8 +303,6 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc); for (i = env->vstart; i < env->vl; i++, env->vstart++) { @@ -301,18 +321,8 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, env->vl, vd, desc, nf, esz, max_elems); } #define GEN_VEXT_LD_STRIDE(NAME, ETYPE, LOAD_FN)\ @@ -359,8 +369,6 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); /* load bytes from guest memory */ for (i = env->vstart; i < evl; i++, env->vstart++) { @@ -372,18 +380,8 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + evl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, evl, vd, desc, nf, esz, max_elems); } /* @@ -484,8 +482,6 @@ vext_ldst_index(void *vd, void *v0, target_ulong base, uint32_t vm = vext_vm(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc); /* load bytes from guest memory */ @@ -505,18 +501,8 @@ vext_ldst_index(void *vd, void *v0,
[PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Commit 752614cab8e6 ("target/riscv: rvv: Add tail agnostic for vector load / store instructions") added code to set the tail elements to 1 in the end of vext_ldst_stride(), vext_ldst_us(), vext_ldst_index() and vext_ldff(). Aside from a env->vl versus an evl value being used in the first loop, the code is being repeated 4 times. Create a helper to avoid code repetition in all those functions. Arguments that are used in the callers (nf, esz and max_elems) are passed as arguments. All other values are being derived inside the helper. Signed-off-by: Daniel Henrique Barboza --- target/riscv/vector_helper.c | 86 +--- 1 file changed, 30 insertions(+), 56 deletions(-) diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c index 00de879787..7d2e3978f1 100644 --- a/target/riscv/vector_helper.c +++ b/target/riscv/vector_helper.c @@ -267,6 +267,28 @@ GEN_VEXT_ST_ELEM(ste_h, int16_t, H2, stw) GEN_VEXT_ST_ELEM(ste_w, int32_t, H4, stl) GEN_VEXT_ST_ELEM(ste_d, int64_t, H8, stq) +static void vext_set_tail_elems_1s(CPURISCVState *env, target_ulong vl, + void *vd, uint32_t desc, uint32_t nf, + uint32_t esz, uint32_t max_elems) +{ +uint32_t total_elems = vext_get_total_elems(env, desc, esz); +uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; +uint32_t vta = vext_vta(desc); +uint32_t registers_used; +int k; + +for (k = 0; k < nf; ++k) { +vext_set_elems_1s(vd, vta, (k * max_elems + vl) * esz, + (k * max_elems + max_elems) * esz); +} + +if (nf * max_elems % total_elems != 0) { +registers_used = ((nf * max_elems) * esz + (vlenb - 1)) / vlenb; +vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, + registers_used * vlenb); +} +} + /* *** stride: access vector element from strided memory */ @@ -281,8 +303,6 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc); for (i = env->vstart; i < env->vl; i++, env->vstart++) { @@ -301,18 +321,8 @@ vext_ldst_stride(void *vd, void *v0, target_ulong base, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, env->vl, vd, desc, nf, esz, max_elems); } #define GEN_VEXT_LD_STRIDE(NAME, ETYPE, LOAD_FN)\ @@ -359,8 +369,6 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, uint32_t nf = vext_nf(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); /* load bytes from guest memory */ for (i = env->vstart; i < evl; i++, env->vstart++) { @@ -372,18 +380,8 @@ vext_ldst_us(void *vd, target_ulong base, CPURISCVState *env, uint32_t desc, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + evl) * esz, - (k * max_elems + max_elems) * esz); -} -if (nf * max_elems % total_elems != 0) { -uint32_t vlenb = env_archcpu(env)->cfg.vlen >> 3; -uint32_t registers_used = -((nf * max_elems) * esz + (vlenb - 1)) / vlenb; -vext_set_elems_1s(vd, vta, (nf * max_elems) * esz, - registers_used * vlenb); -} + +vext_set_tail_elems_1s(env, evl, vd, desc, nf, esz, max_elems); } /* @@ -484,8 +482,6 @@ vext_ldst_index(void *vd, void *v0, target_ulong base, uint32_t vm = vext_vm(desc); uint32_t max_elems = vext_max_elems(desc, log2_esz); uint32_t esz = 1 << log2_esz; -uint32_t total_elems = vext_get_total_elems(env, desc, esz); -uint32_t vta = vext_vta(desc); uint32_t vma = vext_vma(desc); /* load bytes from guest memory */ @@ -505,18 +501,8 @@ vext_ldst_index(void *vd, void *v0, target_ulong base, } } env->vstart = 0; -/* set tail elements to 1s */ -for (k = 0; k < nf; ++k) { -vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz, - (k * max_elems + max_elems) *