1. Patchew
  2. QEMU
  3. [PATCH 0/9] tcg: Use tcg_gen_[s]extract_{i32,i64,tl}
  4. View patch

[RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl

Philippe Mathieu-Daudé posted 9 patches 2 years, 2 months ago
Maintainers: Michael Rolnik <mrolnik@gmail.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Max Filippov <jcmvbkbc@gmail.com>
There is a newer version of this series
[RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl
Posted by Philippe Mathieu-Daudé 2 years, 2 months ago 
Inspired-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: please double-check bits
---
 target/cris/translate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index 65b07e1d80..3a161f8f73 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
      */
     t = tcg_temp_new();
     tcg_gen_shli_tl(d, a, 1);
-    tcg_gen_shli_tl(t, ccs, 31 - 3);
-    tcg_gen_sari_tl(t, t, 31);
+    tcg_gen_sextract_tl(t, ccs, 3, 1);
     tcg_gen_and_tl(t, t, b);
     tcg_gen_add_tl(d, d, t);
 }
-- 
2.41.0
Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl
Posted by Richard Henderson 2 years, 2 months ago 
On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: please double-check bits
> ---
>   target/cris/translate.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index 65b07e1d80..3a161f8f73 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
>        */
>       t = tcg_temp_new();
>       tcg_gen_shli_tl(d, a, 1);
> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
> -    tcg_gen_sari_tl(t, t, 31);
> +    tcg_gen_sextract_tl(t, ccs, 3, 1);

tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);

Also, it appears t_gen_cris_mstep consumes CCS without making sure that it is up-to-date.
Edgar?


r~
Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl
Posted by Philippe Mathieu-Daudé 2 years, 2 months ago 
On 24/10/23 02:26, Richard Henderson wrote:
> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC: please double-check bits
>> ---
>>   target/cris/translate.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>> index 65b07e1d80..3a161f8f73 100644
>> --- a/target/cris/translate.c
>> +++ b/target/cris/translate.c
>> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv 
>> b, TCGv ccs)
>>        */
>>       t = tcg_temp_new();
>>       tcg_gen_shli_tl(d, a, 1);
>> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
>> -    tcg_gen_sari_tl(t, t, 31);
>> +    tcg_gen_sextract_tl(t, ccs, 3, 1);
> 
> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
> 
> Also, it appears t_gen_cris_mstep consumes CCS without making sure that 
> it is up-to-date.

Do you mean we first need to call cris_evaluate_flags?

-- >8 --
diff --git a/target/cris/translate.c b/target/cris/translate.c
index 3a161f8f73..5eb68b8a63 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -177,6 +177,8 @@ static const int preg_sizes[] = {
  #define t_gen_movi_env_TN(member, c) \
      t_gen_mov_env_TN(member, tcg_constant_tl(c))

+static void cris_evaluate_flags(DisasContext *dc);
+
  static inline void t_gen_mov_TN_preg(TCGv tn, int r)
  {
      assert(r >= 0 && r <= 15);
@@ -325,7 +327,7 @@ static void t_gen_cris_dstep(TCGv d, TCGv a, TCGv b)
      tcg_gen_movcond_tl(TCG_COND_GEU, d, d, b, t, d);
  }

-static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
+static void t_gen_cris_mstep(DisasContext *dc, TCGv d, TCGv a, TCGv b, 
TCGv ccs)
  {
      TCGv t;

@@ -335,6 +337,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, 
TCGv ccs)
       *    d += s;
       */
      t = tcg_temp_new();
+    cris_evaluate_flags(dc);
      tcg_gen_shli_tl(d, a, 1);
      tcg_gen_sextract_tl(t, ccs, 3, 1);
      tcg_gen_and_tl(t, t, b);
@@ -702,7 +705,7 @@ static void cris_alu_op_exec(DisasContext *dc, int op,
          t_gen_cris_dstep(dst, a, b);
          break;
      case CC_OP_MSTEP:
-        t_gen_cris_mstep(dst, a, b, cpu_PR[PR_CCS]);
+        t_gen_cris_mstep(dc, dst, a, b, cpu_PR[PR_CCS]);
          break;
      case CC_OP_BOUND:
          tcg_gen_movcond_tl(TCG_COND_LEU, dst, a, b, a, b);
---
Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl
Posted by Philippe Mathieu-Daudé 2 years, 2 months ago 
On 24/10/23 10:53, Philippe Mathieu-Daudé wrote:
> On 24/10/23 02:26, Richard Henderson wrote:
>> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
>>> Inspired-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> RFC: please double-check bits
>>> ---
>>>   target/cris/translate.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>>> index 65b07e1d80..3a161f8f73 100644
>>> --- a/target/cris/translate.c
>>> +++ b/target/cris/translate.c
>>> @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv 
>>> b, TCGv ccs)
>>>        */
>>>       t = tcg_temp_new();
>>>       tcg_gen_shli_tl(d, a, 1);
>>> -    tcg_gen_shli_tl(t, ccs, 31 - 3);
>>> -    tcg_gen_sari_tl(t, t, 31);
>>> +    tcg_gen_sextract_tl(t, ccs, 3, 1);
>>
>> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
>>
>> Also, it appears t_gen_cris_mstep consumes CCS without making sure 
>> that it is up-to-date.
> 
> Do you mean we first need to call cris_evaluate_flags?
> 
> -- >8 --
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index 3a161f8f73..5eb68b8a63 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -177,6 +177,8 @@ static const int preg_sizes[] = {
>   #define t_gen_movi_env_TN(member, c) \
>       t_gen_mov_env_TN(member, tcg_constant_tl(c))
> 
> +static void cris_evaluate_flags(DisasContext *dc);
> +
>   static inline void t_gen_mov_TN_preg(TCGv tn, int r)
>   {
>       assert(r >= 0 && r <= 15);
> @@ -325,7 +327,7 @@ static void t_gen_cris_dstep(TCGv d, TCGv a, TCGv b)
>       tcg_gen_movcond_tl(TCG_COND_GEU, d, d, b, t, d);
>   }
> 
> -static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, TCGv ccs)
> +static void t_gen_cris_mstep(DisasContext *dc, TCGv d, TCGv a, TCGv b, 
> TCGv ccs)
>   {
>       TCGv t;
> 
> @@ -335,6 +337,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b, 
> TCGv ccs)
>        *    d += s;
>        */
>       t = tcg_temp_new();
> +    cris_evaluate_flags(dc);
>       tcg_gen_shli_tl(d, a, 1);
>       tcg_gen_sextract_tl(t, ccs, 3, 1);

Err, to be applied before this patch #8.

>       tcg_gen_and_tl(t, t, b);
> @@ -702,7 +705,7 @@ static void cris_alu_op_exec(DisasContext *dc, int op,
>           t_gen_cris_dstep(dst, a, b);
>           break;
>       case CC_OP_MSTEP:
> -        t_gen_cris_mstep(dst, a, b, cpu_PR[PR_CCS]);
> +        t_gen_cris_mstep(dc, dst, a, b, cpu_PR[PR_CCS]);
>           break;
>       case CC_OP_BOUND:
>           tcg_gen_movcond_tl(TCG_COND_LEU, dst, a, b, a, b);
> ---
>
Re: [RFC PATCH 8/9] target/cris: Use tcg_gen_sextract_tl
Posted by Edgar E. Iglesias 2 years, 2 months ago 
On Tue, Oct 24, 2023 at 2:26 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/23/23 09:09, Philippe Mathieu-Daudé wrote:
> > Inspired-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > RFC: please double-check bits
> > ---
> >   target/cris/translate.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/target/cris/translate.c b/target/cris/translate.c
> > index 65b07e1d80..3a161f8f73 100644
> > --- a/target/cris/translate.c
> > +++ b/target/cris/translate.c
> > @@ -336,8 +336,7 @@ static void t_gen_cris_mstep(TCGv d, TCGv a, TCGv b,
> TCGv ccs)
> >        */
> >       t = tcg_temp_new();
> >       tcg_gen_shli_tl(d, a, 1);
> > -    tcg_gen_shli_tl(t, ccs, 31 - 3);
> > -    tcg_gen_sari_tl(t, t, 31);
> > +    tcg_gen_sextract_tl(t, ccs, 3, 1);


> tcg_gen_sextract_tl(t, ccs, ctz32(N_FLAG), 1);
>

Looks good!

I think the intent was a branch-free version of:
if (ccs & N_FLAG) {
    d += b;
}

Or:
t = ccs & N_FLAG ? UINT32_MAX : 0;
d += b & t;



>
> Also, it appears t_gen_cris_mstep consumes CCS without making sure that it
> is up-to-date.
> Edgar?
>
>
Yes, that looks suspicious!

Best regards,
Edgar


>
> r~
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.