Re: [PATCH v2 6/9] tcg/aarch64: Support TCG_TARGET_SIGNED_ADDR32

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 27 Feb 2022 at 02:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> AArch64 has both sign and zero-extending addressing modes, which
> means that either treatment of guest addresses is equally efficient.
> Enabling this for AArch64 gives us testing of the feature in CI.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/aarch64/tcg-target-sa32.h |  8 +++-
>  tcg/aarch64/tcg-target.c.inc  | 69 +++++++++++++++++++++++------------
>  2 files changed, 52 insertions(+), 25 deletions(-)
>
> diff --git a/tcg/aarch64/tcg-target-sa32.h b/tcg/aarch64/tcg-target-sa32.h
> index cb185b1526..c99e502e4c 100644
> --- a/tcg/aarch64/tcg-target-sa32.h
> +++ b/tcg/aarch64/tcg-target-sa32.h
> @@ -1 +1,7 @@
> -#define TCG_TARGET_SIGNED_ADDR32 0
> +/*
> + * AArch64 has both SXTW and UXTW addressing modes, which means that
> + * it is agnostic to how guest addresses should be represented.
> + * Because aarch64 is more common than the other hosts that will
> + * want to use this feature, enable it for continuous testing.
> + */
> +#define TCG_TARGET_SIGNED_ADDR32 1
> diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
> index 077fc51401..65cab73ea0 100644
> --- a/tcg/aarch64/tcg-target.c.inc
> +++ b/tcg/aarch64/tcg-target.c.inc


>  static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>                              MemOpIdx oi, TCGType ext)
>  {
>      MemOp memop = get_memop(oi);
> -    const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32;
> +    int option = ldst_ext_option();
>
>      /* Byte swapping is left to middle-end expansion. */
>      tcg_debug_assert((memop & MO_BSWAP) == 0);
> @@ -1833,7 +1854,7 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>
>      tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 1);
>      tcg_out_qemu_ld_direct(s, memop, ext, data_reg,
> -                           TCG_REG_X1, otype, addr_reg);
> +                           TCG_REG_X1, option, addr_reg);
>      add_qemu_ldst_label(s, true, oi, ext, data_reg, addr_reg,
>                          s->code_ptr, label_ptr);
>  #else /* !CONFIG_SOFTMMU */
> @@ -1843,10 +1864,10 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>      }
>      if (USE_GUEST_BASE) {
>          tcg_out_qemu_ld_direct(s, memop, ext, data_reg,
> -                               TCG_REG_GUEST_BASE, otype, addr_reg);
> +                               TCG_REG_GUEST_BASE, option, addr_reg);
>      } else {
>          tcg_out_qemu_ld_direct(s, memop, ext, data_reg,
> -                               addr_reg, TCG_TYPE_I64, TCG_REG_XZR);
> +                               addr_reg, option, TCG_REG_XZR);

This doesn't look right. 'option' specifies how we extend the offset
register, but here that is XZR, which is 0 no matter how we choose
to extend it, whereas we aren't going to be extending the base
register 'addr_reg' which is what we do need to either zero or
sign extend. Unfortunately we can't just flip addr_reg and XZR
around, because XZR isn't valid as the base reg.

Is this a pre-existing bug? If addr_reg needs zero extending
we won't be doing that.

>      }
>  #endif /* CONFIG_SOFTMMU */
>  }
> @@ -1855,7 +1876,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>                              MemOpIdx oi)
>  {
>      MemOp memop = get_memop(oi);
> -    const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32;
> +    int option = ldst_ext_option();
>
>      /* Byte swapping is left to middle-end expansion. */
>      tcg_debug_assert((memop & MO_BSWAP) == 0);
> @@ -1866,7 +1887,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>
>      tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 0);
>      tcg_out_qemu_st_direct(s, memop, data_reg,
> -                           TCG_REG_X1, otype, addr_reg);
> +                           TCG_REG_X1, option, addr_reg);
>      add_qemu_ldst_label(s, false, oi, (memop & MO_SIZE)== MO_64,
>                          data_reg, addr_reg, s->code_ptr, label_ptr);
>  #else /* !CONFIG_SOFTMMU */
> @@ -1876,10 +1897,10 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>      }
>      if (USE_GUEST_BASE) {
>          tcg_out_qemu_st_direct(s, memop, data_reg,
> -                               TCG_REG_GUEST_BASE, otype, addr_reg);
> +                               TCG_REG_GUEST_BASE, option, addr_reg);
>      } else {
>          tcg_out_qemu_st_direct(s, memop, data_reg,
> -                               addr_reg, TCG_TYPE_I64, TCG_REG_XZR);
> +                               addr_reg, option, TCG_REG_XZR);
>

Similarly here.

thanks
-- PMM