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

[Richard Henderson]

On 3/3/22 05:04, Peter Maydell wrote:
> >       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.

It's just confusing, because stuff is hidden in macros:

#define USE_GUEST_BASE     (guest_base != 0 || TARGET_LONG_BITS == 32)

We *always* use TCG_REG_GUEST_BASE when we require an extension, so the else case you 
point out will always have option == 3 /* LSL #0 */.

Previously I had a named constant I could use here, but I didn't create names for the full 
'option' field being filled, so I thought it clearer to just pass along the variable. 
Would it be clearer as

    3 /* LSL #0 */

or with some LDST_OPTION_LSL0?


r~