Re: [PATCH 2/6] target/cris: Use hswap_i32() in SWAPW opcode

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.