Commit 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}")
introduced the generic hswap_i32(). Use it instead of open-coding
it as t_gen_swapw().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/cris/translate.c         | 14 +-------------
 target/cris/translate_v10.c.inc |  2 +-
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index 42103b5558..925ed2c6f6 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -399,18 +399,6 @@ static inline void t_gen_swapb(TCGv d, TCGv s)
     tcg_gen_or_tl(d, d, t);
 }
 
-/* Swap the halfwords of the s operand.  */
-static inline void t_gen_swapw(TCGv d, TCGv s)
-{
-    TCGv t;
-    /* d and s refer the same object.  */
-    t = tcg_temp_new();
-    tcg_gen_mov_tl(t, s);
-    tcg_gen_shli_tl(d, t, 16);
-    tcg_gen_shri_tl(t, t, 16);
-    tcg_gen_or_tl(d, d, t);
-}
-
 /*
  * Reverse the bits within each byte.
  *
@@ -1675,7 +1663,7 @@ static int dec_swap_r(CPUCRISState *env, DisasContext *dc)
         tcg_gen_not_tl(t0, t0);
     }
     if (dc->op2 & 4) {
-        t_gen_swapw(t0, t0);
+        tcg_gen_hswap_i32(t0, t0);
     }
     if (dc->op2 & 2) {
         t_gen_swapb(t0, t0);
diff --git a/target/cris/translate_v10.c.inc b/target/cris/translate_v10.c.inc
index b7b0517982..0ff15769ec 100644
--- a/target/cris/translate_v10.c.inc
+++ b/target/cris/translate_v10.c.inc
@@ -506,7 +506,7 @@ static void dec10_reg_swap(DisasContext *dc)
     if (dc->dst & 8)
         tcg_gen_not_tl(t0, t0);
     if (dc->dst & 4)
-        t_gen_swapw(t0, t0);
+        tcg_gen_hswap_i32(t0, t0);
     if (dc->dst & 2)
         t_gen_swapb(t0, t0);
     if (dc->dst & 1)
-- 
2.41.0

On Tue, 22 Aug 2023 at 12:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Commit 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}")
> introduced the generic hswap_i32(). Use it instead of open-coding
> it as t_gen_swapw().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/cris/translate.c         | 14 +-------------
>  target/cris/translate_v10.c.inc |  2 +-
>  2 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index 42103b5558..925ed2c6f6 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -399,18 +399,6 @@ static inline void t_gen_swapb(TCGv d, TCGv s)
>      tcg_gen_or_tl(d, d, t);
>  }
>
> -/* Swap the halfwords of the s operand.  */
> -static inline void t_gen_swapw(TCGv d, TCGv s)
> -{
> -    TCGv t;
> -    /* d and s refer the same object.  */
> -    t = tcg_temp_new();
> -    tcg_gen_mov_tl(t, s);
> -    tcg_gen_shli_tl(d, t, 16);
> -    tcg_gen_shri_tl(t, t, 16);
> -    tcg_gen_or_tl(d, d, t);
> -}
> -
>  /*
>   * Reverse the bits within each byte.
>   *
> @@ -1675,7 +1663,7 @@ static int dec_swap_r(CPUCRISState *env, DisasContext *dc)
>          tcg_gen_not_tl(t0, t0);
>      }
>      if (dc->op2 & 4) {
> -        t_gen_swapw(t0, t0);
> +        tcg_gen_hswap_i32(t0, t0);
>      }
>      if (dc->op2 & 2) {
>          t_gen_swapb(t0, t0);
> diff --git a/target/cris/translate_v10.c.inc b/target/cris/translate_v10.c.inc
> index b7b0517982..0ff15769ec 100644
> --- a/target/cris/translate_v10.c.inc
> +++ b/target/cris/translate_v10.c.inc
> @@ -506,7 +506,7 @@ static void dec10_reg_swap(DisasContext *dc)
>      if (dc->dst & 8)
>          tcg_gen_not_tl(t0, t0);
>      if (dc->dst & 4)
> -        t_gen_swapw(t0, t0);
> +        tcg_gen_hswap_i32(t0, t0);

Both these are operating on TCGv, not TCGv_i32, so I think this
should be tcg_gen_hswap_tl(). (Compare the tcg_gen_not_tl()
calls.)

>      if (dc->dst & 2)
>          t_gen_swapb(t0, t0);
>      if (dc->dst & 1)

thanks
-- PMM

On 22/8/23 13:44, Peter Maydell wrote:
> On Tue, 22 Aug 2023 at 12:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Commit 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}")
>> introduced the generic hswap_i32(). Use it instead of open-coding
>> it as t_gen_swapw().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/cris/translate.c         | 14 +-------------
>>   target/cris/translate_v10.c.inc |  2 +-
>>   2 files changed, 2 insertions(+), 14 deletions(-)


>> diff --git a/target/cris/translate_v10.c.inc b/target/cris/translate_v10.c.inc
>> index b7b0517982..0ff15769ec 100644
>> --- a/target/cris/translate_v10.c.inc
>> +++ b/target/cris/translate_v10.c.inc
>> @@ -506,7 +506,7 @@ static void dec10_reg_swap(DisasContext *dc)
>>       if (dc->dst & 8)
>>           tcg_gen_not_tl(t0, t0);
>>       if (dc->dst & 4)
>> -        t_gen_swapw(t0, t0);
>> +        tcg_gen_hswap_i32(t0, t0);
> 
> Both these are operating on TCGv, not TCGv_i32, so I think this
> should be tcg_gen_hswap_tl(). (Compare the tcg_gen_not_tl()
> calls.)

You are correct, if someone copies part of this code to a new
function compiled for a 64-bit target, this won't build.

We know cris is a 32-bit only target.

When implementing tcg_gen_foo_tl(), should we implement both
corresponding tcg_gen_foo_i32/i64() even if one is never used
(thus not tested)?

I like completeness, but I'm a bit reluctant to commit unused
code (mostly for maintenance burden).

Maybe I can go mid-way and only add tcg_gen_hswap_tl() ->
tcg_gen_hswap_i32() here. If tcg_gen_hswap_tl() were used on
a 64-bit target then we'd get a build failure. Does that
sound reasonable?

Thanks,

Phil.

On Tue, 22 Aug 2023 at 14:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 22/8/23 13:44, Peter Maydell wrote:
> > On Tue, 22 Aug 2023 at 12:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Commit 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}")
> >> introduced the generic hswap_i32(). Use it instead of open-coding
> >> it as t_gen_swapw().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   target/cris/translate.c         | 14 +-------------
> >>   target/cris/translate_v10.c.inc |  2 +-
> >>   2 files changed, 2 insertions(+), 14 deletions(-)
>
>
> >> diff --git a/target/cris/translate_v10.c.inc b/target/cris/translate_v10.c.inc
> >> index b7b0517982..0ff15769ec 100644
> >> --- a/target/cris/translate_v10.c.inc
> >> +++ b/target/cris/translate_v10.c.inc
> >> @@ -506,7 +506,7 @@ static void dec10_reg_swap(DisasContext *dc)
> >>       if (dc->dst & 8)
> >>           tcg_gen_not_tl(t0, t0);
> >>       if (dc->dst & 4)
> >> -        t_gen_swapw(t0, t0);
> >> +        tcg_gen_hswap_i32(t0, t0);
> >
> > Both these are operating on TCGv, not TCGv_i32, so I think this
> > should be tcg_gen_hswap_tl(). (Compare the tcg_gen_not_tl()
> > calls.)
>
> You are correct, if someone copies part of this code to a new
> function compiled for a 64-bit target, this won't build.
>
> We know cris is a 32-bit only target.
>
> When implementing tcg_gen_foo_tl(), should we implement both
> corresponding tcg_gen_foo_i32/i64() even if one is never used
> (thus not tested)?
>
> I like completeness, but I'm a bit reluctant to commit unused
> code (mostly for maintenance burden).
>
> Maybe I can go mid-way and only add tcg_gen_hswap_tl() ->
> tcg_gen_hswap_i32() here. If tcg_gen_hswap_tl() were used on
> a 64-bit target then we'd get a build failure. Does that
> sound reasonable?

We already have tcg_gen_hswap_tl (it's a #define like all the
_tl symbols), so I'm just asking that you use it rather than
the _i32 version. If we were writing the cris target code
from scratch these days we'd probably write it to use _i32
throughout, but since it's not written that way I think
it's better to continue the pattern rather than deviate
from it.

thanks
-- PMM

On 22/8/23 15:27, Peter Maydell wrote:
> On Tue, 22 Aug 2023 at 14:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 22/8/23 13:44, Peter Maydell wrote:
>>> On Tue, 22 Aug 2023 at 12:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> Commit 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}")
>>>> introduced the generic hswap_i32(). Use it instead of open-coding
>>>> it as t_gen_swapw().
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    target/cris/translate.c         | 14 +-------------
>>>>    target/cris/translate_v10.c.inc |  2 +-
>>>>    2 files changed, 2 insertions(+), 14 deletions(-)
>>
>>
>>>> diff --git a/target/cris/translate_v10.c.inc b/target/cris/translate_v10.c.inc
>>>> index b7b0517982..0ff15769ec 100644
>>>> --- a/target/cris/translate_v10.c.inc
>>>> +++ b/target/cris/translate_v10.c.inc
>>>> @@ -506,7 +506,7 @@ static void dec10_reg_swap(DisasContext *dc)
>>>>        if (dc->dst & 8)
>>>>            tcg_gen_not_tl(t0, t0);
>>>>        if (dc->dst & 4)
>>>> -        t_gen_swapw(t0, t0);
>>>> +        tcg_gen_hswap_i32(t0, t0);
>>>
>>> Both these are operating on TCGv, not TCGv_i32, so I think this
>>> should be tcg_gen_hswap_tl(). (Compare the tcg_gen_not_tl()
>>> calls.)
>>
>> You are correct, if someone copies part of this code to a new
>> function compiled for a 64-bit target, this won't build.
>>
>> We know cris is a 32-bit only target.
>>
>> When implementing tcg_gen_foo_tl(), should we implement both
>> corresponding tcg_gen_foo_i32/i64() even if one is never used
>> (thus not tested)?
>>
>> I like completeness, but I'm a bit reluctant to commit unused
>> code (mostly for maintenance burden).
>>
>> Maybe I can go mid-way and only add tcg_gen_hswap_tl() ->
>> tcg_gen_hswap_i32() here. If tcg_gen_hswap_tl() were used on
>> a 64-bit target then we'd get a build failure. Does that
>> sound reasonable?
> 
> We already have tcg_gen_hswap_tl (it's a #define like all the
> _tl symbols), so I'm just asking that you use it rather than
> the _i32 version. If we were writing the cris target code
> from scratch these days we'd probably write it to use _i32
> throughout, but since it's not written that way I think
> it's better to continue the pattern rather than deviate
> from it.

Doh I missed commit 46be8425ff also added tcg_gen_hswap_tl()...

Thanks!

Phil.