Re: [PATCH v2 23/30] tcg/loongarch64: Add softmmu load/store helpers, implement qemu_ld/qemu_st ops

[WANG Xuerui <i.qemu@xen0n.name>]

Hi Richard,

On 9/23/21 00:29, Richard Henderson wrote:
> On 9/21/21 1:19 PM, WANG Xuerui wrote:
>> +    /* Compare masked address with the TLB entry.  */
>> +    label_ptr[0] = s->code_ptr;
>> +    tcg_out_opc_bne(s, TCG_REG_TMP0, TCG_REG_TMP1, 0);
>> +
>> +    /* TLB Hit - translate address using addend.  */
>> +    tcg_out_opc_add_d(s, TCG_REG_TMP0, TCG_REG_TMP2, addrl);
>
> You removed a little too much here.  You still need
>
>     if (TARGET_LONG_BITS == 32) {
>         tcg_out_ext32u(s, TCG_REG_TMP0, addrl);
>         addrl = TCG_REG_TMP0;
>     }
>     tcg_out_opc_add_d(s, TCG_REG_TMP0, TCG_REG_TMP2, addrl);
Ah, of course that was necessary because TARGET_LONG_BITS depends on the 
*target*... silly me. Thanks for catching this, I was just about to 
debug the "mysterious" segfaults on all the 32-bit targets :facepalm:
>
>> +static void add_qemu_ldst_label(TCGContext *s, int is_ld, 
>> TCGMemOpIdx oi,
>> +                                TCGReg datalo, TCGReg addrlo,
>> +                                void *raddr, tcg_insn_unit **label_ptr)
>> +{
>> +    TCGLabelQemuLdst *label = new_ldst_label(s);
>> +
>> +    label->is_ld = is_ld;
>> +    label->oi = oi;
>> +    label->type = 0;
>
> Type should be set based on "is_64" argument to tcg_out_qemu_ld (or 
> indeed, is_64 could be replaced by "type", which would probably make 
> more sense).
>
> This will be used to fix...
>
>> +    if (opc & MO_SIGN) {
>> +        /* Sign-extend directly into destination.  */
>> +        switch (size) {
>> +        case MO_8:
>> +            tcg_out_ext8s(s, l->datalo_reg, TCG_REG_A0);
>> +            break;
>> +        case MO_16:
>> +            tcg_out_ext16s(s, l->datalo_reg, TCG_REG_A0);
>> +            break;
>> +        case MO_32:
>> +            tcg_out_ext32s(s, l->datalo_reg, TCG_REG_A0);
>> +            break;
>> +        default:
>> +            g_assert_not_reached();
>> +            break;
>> +        }
>> +    } else {
>> +        tcg_out_mov(s, size == MO_64, l->datalo_reg, TCG_REG_A0);
>> +    }
>
> ... this, where TCG_TYPE_I32 loads should always be sign-extended from 
> 32-bits.  Something like
>
>     switch (opc & MO_SSIZE) {
>     case MO_SB:
>         ext8s;
>         break;
>     case MO_SH:
>         ext16s;
>         break;
>     case MO_SL:
>         ext32s;
>         break;
>     case MO_UL:
>         if (type == TCG_TYPE_I32) {
>             ext32s;
>             break;
>         }
>         /* fall through */
>     default:
>         tcg_out_mov(s, TCG_TYPE_REG, datalo, A0);
>         break;
>     }
Ack; I'll modify tcg_out_qemu_ld and add_qemu_ldst_label to take a TCGType.
>
>> +    case MO_64:
>> +        tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_A2, l->datalo_reg);
>
> TCG_TYPE_I64, to match MO_64.
Ack; will fix.
>
>> +    if (USE_GUEST_BASE) {
>> +        tcg_out_opc_add_d(s, base, TCG_GUEST_BASE_REG, addr_regl);
>> +    } else {
>> +        tcg_out_opc_add_d(s, base, addr_regl, TCG_REG_ZERO);
>> +    }
>
> Still adding zero in tcg_out_qemu_st.
One probably should not even attempt to fix code at 4 am, esp. while 
going through tens of review comments...
>
>
> r~