[PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()

Daniel Henrique Barboza posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230221184525.140704-1-dbarboza@ventanamicro.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>
target/riscv/vector_helper.c | 86 +++++++++++++-----------------------
1 file changed, 30 insertions(+), 56 deletions(-)
[PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Posted by Daniel Henrique Barboza 1 year, 2 months ago
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 <dbarboza@ventanamicro.com>
---
 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) * 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_INDEX(NAME, ETYPE, INDEX_FN, LOAD_FN)                  \
@@ -585,8 +571,6 @@ vext_ldff(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);
     target_ulong addr, offset, remain;
 
@@ -647,18 +631,8 @@ ProbeSuccess:
         }
     }
     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_LDFF(NAME, ETYPE, LOAD_FN)               \
-- 
2.39.2
Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Posted by Daniel Henrique Barboza 1 year, 2 months ago
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 <dbarboza@ventanamicro.com>
> ---
>   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) * 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_INDEX(NAME, ETYPE, INDEX_FN, LOAD_FN)                  \
> @@ -585,8 +571,6 @@ vext_ldff(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);
>       target_ulong addr, offset, remain;
>   
> @@ -647,18 +631,8 @@ ProbeSuccess:
>           }
>       }
>       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_LDFF(NAME, ETYPE, LOAD_FN)               \
Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Posted by Frank Chang 1 year, 2 months ago
Reviewed-by: Frank Chang <frank.chang@sifive.com>

On Wed, Feb 22, 2023 at 2:46 AM Daniel Henrique Barboza <
dbarboza@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 <dbarboza@ventanamicro.com>
> ---
>  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) * 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_INDEX(NAME, ETYPE, INDEX_FN, LOAD_FN)
>   \
> @@ -585,8 +571,6 @@ vext_ldff(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);
>      target_ulong addr, offset, remain;
>
> @@ -647,18 +631,8 @@ ProbeSuccess:
>          }
>      }
>      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_LDFF(NAME, ETYPE, LOAD_FN)               \
> --
> 2.39.2
>
>
>
Re: [PATCH] target/riscv/vector_helper.c: create vext_set_tail_elems_1s()
Posted by liweiwei 1 year, 2 months ago
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 <dbarboza@ventanamicro.com>

LGTM.

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

> ---
>   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, 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_INDEX(NAME, ETYPE, INDEX_FN, LOAD_FN)                  \
> @@ -585,8 +571,6 @@ vext_ldff(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);
>       target_ulong addr, offset, remain;
>   
> @@ -647,18 +631,8 @@ ProbeSuccess:
>           }
>       }
>       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_LDFF(NAME, ETYPE, LOAD_FN)               \