Re: [PATCH v2 03/24] target/arm: Adjust gen_aa32_{ld, st}_i32 for align+endianness

[Peter Maydell]

On Tue, 8 Dec 2020 at 18:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Create a finalize_memop function that computes alignment and
> endianness and returns the final MemOp for the operation.
>
> Split out gen_aa32_{ld,st}_internal_i32 which bypasses any special
> handling of endianness or alignment.  Adjust gen_aa32_{ld,st}_i32
> so that s->be_data is not added by the callers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.h          |  24 ++++++++
>  target/arm/translate.c          | 100 +++++++++++++++++---------------
>  target/arm/translate-neon.c.inc |   9 +--
>  3 files changed, 79 insertions(+), 54 deletions(-)
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index fb66b4d8a0..22a4b15d45 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -448,4 +448,28 @@ static inline TCGv_ptr fpstatus_ptr(ARMFPStatusFlavour 
> flavour)
>      return statusptr;
>  }
>
> +/**
> + * finalize_memop:
> + * @s: DisasContext
> + * @opc: size+sign+align of the memory operation
> + *
> + * Build the complete MemOp for a memory operation, including alignment
> + * and endianness.
> + *
> + * If (op & MO_AMASK) then the operation already contains the required
> + * alignment, e.g. for AccType_ATOMIC.  Otherwise, this an optionally
> + * unaligned operation, e.g. for AccType_NORMAL.
> + *
> + * In the later case, there are configuration bits that require alignment,

"latter case".

> + * and this is applied here.  Note that there is no way to indicate that
> + * no alignment should ever be enforced; this must be handled manually.
> + */
> +static inline MemOp finalize_memop(DisasContext *s, MemOp opc)
> +{
> +    if (s->align_mem && !(opc & MO_AMASK)) {
> +        opc |= MO_ALIGN;
> +    }
> +    return opc | s->be_data;
> +}
> +

> +#define DO_GEN_LD(SUFF, OPC)                                            \
> +    static inline void gen_aa32_ld##SUFF(DisasContext *s, TCGv_i32 val, \
> +                                         TCGv_i32 a32, int index)       \
> +    {                                                                   \
> +        gen_aa32_ld_i32(s, val, a32, index, OPC);                       \
>      }

> +#define DO_GEN_ST(SUFF, OPC)                                            \
> +    static inline void gen_aa32_st##SUFF(DisasContext *s, TCGv_i32 val, \
> +                                         TCGv_i32 a32, int index)       \
> +    {                                                                   \
> +        gen_aa32_st_i32(s, val, a32, index, OPC);                       \
> +    }

Since these generated functions no longer do anything extra
that the gen_aa32_{ld,st}_i32() that the call don't do,
we could reasonably have a follow-on patch that makes all
the callsites directly call those functions and remove
the extra layer of indirection, I think. The main reason
we had them before was so that we had somewhere to add
the handling of s->be_data.

For this patch, other than the typo above,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM