Re: [PATCH v2 3/7] target/s390x: vxeh2: vector shift {double by bit, left, right {logical,arithmetic}}

[Richard Henderson]

On 3/6/22 16:03, David Miller wrote:
>   }
> +/**
> + * deposit8:
> + * @value: initial value to insert bit field into
> + * @start: the lowest bit in the bit field (numbered from 0)
> + * @length: the length of the bit field
> + * @fieldval: the value to insert into the bit field
> + *
> + * Deposit @fieldval into the 8 bit @value at the bit field specified
> + * by the @start and @length parameters, and return the modified
> + * @value. Bits of @value outside the bit field are not modified.
> + * Bits of @fieldval above the least significant @length bits are
> + * ignored. The bit field must lie entirely within the 8 bit byte.
> + * It is valid to request that all 8 bits are modified (ie @length
> + * 8 and @start 0).
> + *
> + * Returns: the modified @value.
> + */
> +static inline uint8_t deposit8(uint8_t value, int start, int length,
> +                               uint8_t fieldval)
> +{
> +    uint8_t mask;
> +    assert(start >= 0 && length > 0 && length <= 8 - start);
> +    mask = (~0ULL >> (8 - length)) << start;
> +    return (value & ~mask) | ((fieldval << start) & mask);
> +}
>   

(1) must be a separate patch.
(2) watch the whitespace at the top.

Given we have extract8 already, this is indeed missing.
But I'm surprised you'd need this...

Also, this is still doing too much.
Changes to existing instructions should not be mixed with new instructions.


>  static DisasJumpType op_vsl(DisasContext *s, DisasOps *o)
>  {
> -    TCGv_i64 shift = tcg_temp_new_i64();
> -
> -    read_vec_element_i64(shift, get_field(s, v3), 7, ES_8);
> -    if (s->fields.op2 == 0x74) {
> -        tcg_gen_andi_i64(shift, shift, 0x7);
> +    const bool B = 0x75 == s->fields.op2;

I really don't like testing opcodes after the fact.  This is the job for insn-data.def. 
Either pass in data with the DATA element of F(), or use a helper function.

> +static DisasJumpType op_vsld(DisasContext *s, DisasOps *o)
>  {
> -    const uint8_t i4 = get_field(s, i4) & 0xf;
> -    const int left_shift = (i4 & 7) * 8;
> -    const int right_shift = 64 - left_shift;
> +    const uint8_t mask = (0x86 == s->fields.op2) ? 7 : 15;
> +    const uint8_t mul  = (0x86 == s->fields.op2) ? 1 : 8;
> +    const uint8_t i4   = get_field(s, i4);
> +    const int shift = 64 - (i4 & 7) * mul;
> +
> +    if (i4 & ~mask) {
> +        gen_program_exception(s, PGM_SPECIFICATION);
> +        return DISAS_NORETURN;
> +    }
>      TCGv_i64 t0 = tcg_temp_new_i64();
>      TCGv_i64 t1 = tcg_temp_new_i64();
>      TCGv_i64 t2 = tcg_temp_new_i64();
> @@ -2053,8 +2060,8 @@ static DisasJumpType op_vsldb(DisasContext *s, DisasOps 
> *o)
>          read_vec_element_i64(t1, get_field(s, v3), 0, ES_64);
>          read_vec_element_i64(t2, get_field(s, v3), 1, ES_64);
>      }
> -    tcg_gen_extract2_i64(t0, t1, t0, right_shift);
> -    tcg_gen_extract2_i64(t1, t2, t1, right_shift);
> +    tcg_gen_extract2_i64(t0, t1, t0, shift);
> +    tcg_gen_extract2_i64(t1, t2, t1, shift);

The renaming of right_shift to shift is probably misleading, since extract2 *always* 
performs a right-shift.

> +    tcg_gen_extract2_i64(t0, t1, t0, left_shift);
> +    tcg_gen_extract2_i64(t1, t2, t1, left_shift);

Which makes this bit from op_vsrd actively misleading (though the code appears to be 
correct, its just the variable name that's wrong).

> +void HELPER(gvec_vsl_ve2)(void *v1, const void *v2, const void *v3,
> +                          uint32_t desc)
> +{
> +    uint8_t i, v;
> +    S390Vector tmp = {};
> +    for (i = 0; i < 16; i++) {
> +        const uint8_t shift = s390_vec_read_element8(v3, i) & 7;
> +        v = s390_vec_read_element8(v2, i);
> +
> +        if (shift) {
> +            v <<= shift;
> +            if (i < 15) {
> +                v |= extract8(s390_vec_read_element8(v2, i + 1),
> +                              8 - shift, shift);
> +            }

Possibly better as

    if (shift) {
        uint16_t tmp = (uint16_t)v << 8;
        if (i < 15) {
            tmp |= s390_vec_read_element8(v2, i + 1);
        }
        tmp <<= shift;
        v = tmp >> 8;
    }

Similarly for the right shifts.

I wonder if it's worth checking that the values are identical, so that we can use the 
original vsl implementation, using double-word shifts.  E.g.

    uint64_t v3_0 = s390_vec_read_element64(v3, 0);
    uint64_t v3_1 = s390_vec_read_element64(v3, 1);
    uint64_t sh_0 = dup_const(MO_8, v3_0 & 7);
    uint64_t sh_m = dup_const(MO_8, 7);

    if ((v3_0 & sh_m) == sh_0 && (v3_1 & sh_m) == sh_0) {
        helper_gvec_vsrl(v1, v2, v3, desc);
        return;
    }


r~