| Message ID | 20230822110129.41022-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target: Use TCG generic gen_hswap_i32/i64() | expand |
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.
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)
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(-)