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

[Peter Maydell]

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