Re: [PATCH v9 02/10] target/riscv: handle vstart >= vl in vext_set_tail_elems_1s()

[Richard Henderson]

On 3/9/24 10:43, Daniel Henrique Barboza wrote:
> We're going to make changes that will required each helper to be
> responsible for the 'vstart' management, i.e. we will relieve the
> 'vstart < vl' assumption that helpers have today.
>
> To do that we'll need to deal with how we're updating tail elements
> first. We can't update them if vstart >= vl, but at this moment we're
> not guarding for it.
>
> We have the vext_set_tail_elems_1s() helper to update tail elements.
> Change it to accept an 'env' pointer, where we can read both vstart and
> vl, and make it a no-op if vstart >= vl. Note that callers will need to
> set env->start = 0 *after* the helper from now on.
>
> The exception are three helpers: vext_ldst_stride(), vext_ldst_us() and
> vext_ldst_index(). They are are incrementing env->vstart during
> execution and will end up with env->vstart = vl when tail updating. For
> these cases only, do an early check and exit if vstart >= vl, and set
> env->vstart = 0 before updating the tail.
>
> For everyone else we'll do vext_set_tail_elems_1s() and then clear
> env->vstart. This is the case of vext_ldff() that is already using
> set_tail_elems_1s(), and will be the case for the rest after the next
> patches.
>
> Let's also simplify the API a little by removing the 'nf' argument since
> it can be derived from 'desc'.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/vector_helper.c | 59 ++++++++++++++++++++++++++++++------
>   1 file changed, 49 insertions(+), 10 deletions(-)

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

> +    uint32_t nf = vext_nf(desc);
>       int k;
>   
> -    if (vta == 0) {
> +    /*
> +     * Section 5.4 of the RVV spec mentions:
> +     * "When vstart ≥ vl, there are no body elements, and no
> +     *  elements are updated in any destination vector register
> +     *  group, including that no tail elements are updated
> +     *  with agnostic values."
> +     */
> +    if (vta == 0 || env->vstart >= env->vl) {
>           return;
>       }
>   
>       for (k = 0; k < nf; ++k) {

Existing issue, and we know nf <= 8, but bad form to mix signs on the 
comparison.

> -        vext_set_elems_1s(vd, vta, (k * max_elems + vl) * esz,
> +        vext_set_elems_1s(vd, vta, (k * max_elems + env->vl) * esz,

You may wish to hoist vl to a local anyway.


r~