[v3,11/21] target/riscv: support for 128-bit bitwise instructions

The 128-bit bitwise instructions do not need any function prototype change
as the functions can be applied independently on the lower and upper part of
the registers.

Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
Co-authored-by: Fabien Portas <fabien.portas@grenoble-inp.org>
---
 target/riscv/translate.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

On 10/19/21 2:48 AM, Frédéric Pétrot wrote:
> The 128-bit bitwise instructions do not need any function prototype change
> as the functions can be applied independently on the lower and upper part of
> the registers.
> 
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> Co-authored-by: Fabien Portas <fabien.portas@grenoble-inp.org>
> ---
>   target/riscv/translate.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index e8f08f921e..71982f6284 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -429,6 +429,17 @@ static bool gen_logic_imm_fn(DisasContext *ctx, arg_i *a, DisasExtend ext,
>   
>       gen_set_gpr(ctx, a->rd, dest);
>   
> +    if (get_xl_max(ctx) == MXL_RV128) {
> +        if (get_ol(ctx) ==  MXL_RV128) {
> +            uint64_t immh = -(a->imm < 0);
> +            src1 = get_gprh(ctx, a->rs1);
> +            dest = dest_gprh(ctx, a->rd);
> +
> +            func(dest, src1, immh);
> +        }
> +        gen_set_gprh(ctx, a->rd, dest);
> +    }

If ol < RV128, you're storing the low dest into the gprh, which is wrong.  It should be 
the sign-extension of the low part.  But that should happen for all writes.

Earlier, I suggested gen_set_gpr128 instead of gen_set_gprh.
I think this should be written

     if (get_xl(ctx) == MXL_RV128) {
         TCGv src1h = get_gprh(ctx, a->rs1);
         TCGv desth = dest_gprh(ctx, a->rd);

         func(dest, src1h, -(a->imm < 0));
         gen_set_gpr128(ctx, a->rd, dest, desth);
     } else {
         gen_set_gpr(ctx, a->rd, dest);
     }

Where gen_set_gpr will handle the sign-extension to 128-bits.


> @@ -443,6 +454,17 @@ static bool gen_logic(DisasContext *ctx, arg_r *a, DisasExtend ext,
>   
>       gen_set_gpr(ctx, a->rd, dest);
>   
> +    if (get_xl_max(ctx) == MXL_RV128) {
> +        if (get_ol(ctx) ==  MXL_RV128) {
> +            dest = dest_gprh(ctx, a->rd);
> +            src1 = get_gprh(ctx, a->rs1);
> +            src2 = get_gprh(ctx, a->rs2);
> +
> +            func(dest, src1, src2);
> +        }
> +        gen_set_gprh(ctx, a->rd, dest);
> +    }

Similarly.


r~

Le 20/10/2021 à 19:47, Richard Henderson a écrit :
> On 10/19/21 2:48 AM, Frédéric Pétrot wrote:
>> The 128-bit bitwise instructions do not need any function prototype change
>> as the functions can be applied independently on the lower and upper part of
>> the registers.
>>
>> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
>> Co-authored-by: Fabien Portas <fabien.portas@grenoble-inp.org>
>> ---
>>   target/riscv/translate.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index e8f08f921e..71982f6284 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -429,6 +429,17 @@ static bool gen_logic_imm_fn(DisasContext *ctx, arg_i *a,
>> DisasExtend ext,
>>         gen_set_gpr(ctx, a->rd, dest);
>>   +    if (get_xl_max(ctx) == MXL_RV128) {
>> +        if (get_ol(ctx) ==  MXL_RV128) {
>> +            uint64_t immh = -(a->imm < 0);
>> +            src1 = get_gprh(ctx, a->rs1);
>> +            dest = dest_gprh(ctx, a->rd);
>> +
>> +            func(dest, src1, immh);
>> +        }
>> +        gen_set_gprh(ctx, a->rd, dest);
>> +    }
> 
> If ol < RV128, you're storing the low dest into the gprh, which is wrong.  It
> should be the sign-extension of the low part.  But that should happen for all
> writes.

  Thanks for your feedback (on the other parts too) that I'll apply.

  On this specific case, in gen_set_gprh I check that the operation is not on
  128 bit in which case I propagate the sign of the low part instead of using
  dest (the spec says that the sign should propagate to misa_xl_max, irrelevant
  of xl).
  This implicitly forces the order in which the functions must be called as you
  noticed, and introducing a higher level function as you suggest would indeed
  make things more readable, and this can probably be applied in most places.

Frédéric