Re: [PATCH 11/37] accel/tcg: Implement translator_ld*_end

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/12/25 20:44, Richard Henderson wrote:
> Add a new family of translator load functions which take
> an absolute endianness value in the form of MO_BE/MO_LE.
> Expand the other translator_ld* functions on top of this.
> Remove exec/tswap.h from translator.c.
> 

Is there a need further down the road to break the dependency to tswap?
I am not sure of the benefit to drop tswap, as the resulting code is 
more complicated than simply calling tswap*().

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/translator.h | 49 ++++++++++++++++++++++++---------------
>   accel/tcg/translator.c    | 26 +++++++++++++++------
>   2 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 205dd85bba..3c32655569 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -18,7 +18,7 @@
>    * member in your target-specific DisasContext.
>    */
>   
> -#include "qemu/bswap.h"
> +#include "exec/memop.h"
>   #include "exec/vaddr.h"
>   
>   /**
> @@ -181,42 +181,53 @@ bool translator_io_start(DisasContextBase *db);
>    */
>   
>   uint8_t translator_ldub(CPUArchState *env, DisasContextBase *db, vaddr pc);
> -uint16_t translator_lduw(CPUArchState *env, DisasContextBase *db, vaddr pc);
> -uint32_t translator_ldl(CPUArchState *env, DisasContextBase *db, vaddr pc);
> -uint64_t translator_ldq(CPUArchState *env, DisasContextBase *db, vaddr pc);
> +uint16_t translator_lduw_end(CPUArchState *env, DisasContextBase *db,
> +                             vaddr pc, MemOp endian);
> +uint32_t translator_ldl_end(CPUArchState *env, DisasContextBase *db,
> +                            vaddr pc, MemOp endian);
> +uint64_t translator_ldq_end(CPUArchState *env, DisasContextBase *db,
> +                            vaddr pc, MemOp endian);
> +
> +#ifdef COMPILING_PER_TARGET
> +static inline uint16_t
> +translator_lduw(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +{
> +    return translator_lduw_end(env, db, pc, MO_TE);
> +}
> +
> +static inline uint32_t
> +translator_ldl(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +{
> +    return translator_ldl_end(env, db, pc, MO_TE);
> +}
> +
> +static inline uint64_t
> +translator_ldq(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +{
> +    return translator_ldq_end(env, db, pc, MO_TE);
> +}
>   
>   static inline uint16_t
>   translator_lduw_swap(CPUArchState *env, DisasContextBase *db,
>                        vaddr pc, bool do_swap)
>   {
> -    uint16_t ret = translator_lduw(env, db, pc);
> -    if (do_swap) {
> -        ret = bswap16(ret);
> -    }
> -    return ret;
> +    return translator_lduw_end(env, db, pc, MO_TE ^ (do_swap * MO_BSWAP));
>   }
>   
>   static inline uint32_t
>   translator_ldl_swap(CPUArchState *env, DisasContextBase *db,
>                       vaddr pc, bool do_swap)
>   {
> -    uint32_t ret = translator_ldl(env, db, pc);
> -    if (do_swap) {
> -        ret = bswap32(ret);
> -    }
> -    return ret;
> +    return translator_ldl_end(env, db, pc, MO_TE ^ (do_swap * MO_BSWAP));
>   }
>   
>   static inline uint64_t
>   translator_ldq_swap(CPUArchState *env, DisasContextBase *db,
>                       vaddr pc, bool do_swap)
>   {
> -    uint64_t ret = translator_ldq(env, db, pc);
> -    if (do_swap) {
> -        ret = bswap64(ret);
> -    }
> -    return ret;
> +    return translator_ldq_end(env, db, pc, MO_TE ^ (do_swap * MO_BSWAP));
>   }
> +#endif /* COMPILING_PER_TARGET */
>   
>   /**
>    * translator_fake_ld - fake instruction load
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 64fa069b51..405e0b44c4 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -8,13 +8,13 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>   #include "qemu/log.h"
>   #include "qemu/error-report.h"
>   #include "exec/exec-all.h"
>   #include "exec/cpu-ldst-common.h"
>   #include "exec/translator.h"
>   #include "exec/plugin-gen.h"
> -#include "exec/tswap.h"
>   #include "tcg/tcg-op-common.h"
>   #include "internal-target.h"
>   #include "disas/disas.h"
> @@ -467,7 +467,8 @@ uint8_t translator_ldub(CPUArchState *env, DisasContextBase *db, vaddr pc)
>       return val;
>   }
>   
> -uint16_t translator_lduw(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +uint16_t translator_lduw_end(CPUArchState *env, DisasContextBase *db,
> +                             vaddr pc, MemOp endian)
>   {
>       uint16_t val;
>   
> @@ -476,10 +477,14 @@ uint16_t translator_lduw(CPUArchState *env, DisasContextBase *db, vaddr pc)
>           val = cpu_ldw_code_mmu(env, pc, oi, 0);
>           record_save(db, pc, &val, sizeof(val));
>       }
> -    return tswap16(val);
> +    if (endian & MO_BSWAP) {
> +        val = bswap16(val);
> +    }
> +    return val;
>   }
>   
> -uint32_t translator_ldl(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +uint32_t translator_ldl_end(CPUArchState *env, DisasContextBase *db,
> +                            vaddr pc, MemOp endian)
>   {
>       uint32_t val;
>   
> @@ -488,10 +493,14 @@ uint32_t translator_ldl(CPUArchState *env, DisasContextBase *db, vaddr pc)
>           val = cpu_ldl_code_mmu(env, pc, oi, 0);
>           record_save(db, pc, &val, sizeof(val));
>       }
> -    return tswap32(val);
> +    if (endian & MO_BSWAP) {
> +        val = bswap32(val);
> +    }
> +    return val;
>   }
>   
> -uint64_t translator_ldq(CPUArchState *env, DisasContextBase *db, vaddr pc)
> +uint64_t translator_ldq_end(CPUArchState *env, DisasContextBase *db,
> +                            vaddr pc, MemOp endian)
>   {
>       uint64_t val;
>   
> @@ -500,7 +509,10 @@ uint64_t translator_ldq(CPUArchState *env, DisasContextBase *db, vaddr pc)
>           val = cpu_ldq_code_mmu(env, pc, oi, 0);
>           record_save(db, pc, &val, sizeof(val));
>       }
> -    return tswap64(val);
> +    if (endian & MO_BSWAP) {
> +        val = bswap64(val);
> +    }
> +    return val;
>   }
>   
>   void translator_fake_ld(DisasContextBase *db, const void *data, size_t len)