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

[Philippe Mathieu-Daudé]

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.