Re: [Qemu-devel] [PATCH] Porting TCG to alpha platform

[Richard Henderson <rth@twiddle.net>]

On 01/22/2010 07:47 AM, identifier scorpio wrote:
> Is there any good method to find the bug except "guess and try"?

-singlestep -d in_asm,cpu,exec

Use these options on both Alpha host and x86_64 host and diff the
resulting output files.  That will show you which target instruction is
emulated differently.

> And it seems that few people is interested in porting QEMU/TCG to
> alpha platform, why? just because alpha machine is disappearing?

Probably.  I no longer have functioning alpha hardware, which is why 
I've become interested in qemu with alpha as a target.

> +    tcg_out32(s, (opc)|INSN_RA(ra)|INSN_DISP21(disp));

Coding style: spaces around operators, no useless parens around opc.

> +static inline int tcg_target_const_match( \
> +    tcg_target_long val, const TCGArgConstraint *arg_ct)

Coding style: no trailing \; there's no need for line continuation 
anywhere except in preprocessor directives.  Merge the first argument 
into the first line, assuming that can happen without exceeding 80 
columns.  Arguments that do get split to subsequent lines should be 
properly indented below the first argument on the previous line.

> +    if ( type == TCG_TYPE_I32)
> +        val = (int32_t)val;

Coding style: use braces, even for a single statement.  Yes, there's 
plenty of code in the project that doesn't, but the coding style is 
being enforced on all new code.

Also, extra space after open paren.

> +    if (disp != (int16_t)disp) {
> +        tcg_out_movi(s, TCG_TYPE_I64, TMP_REG1, disp);
> +        tcg_out_fmt_opr(s, INSN_ADDQ, rb, TMP_REG1, TMP_REG1);
> +        tcg_out_fmt_mem(s, opc, ra, TMP_REG1, 0);
> +    } else {
> +        tcg_out_fmt_mem(s, opc, ra, rb, disp);
> +    }

The reason I suggested writing the tcg_out_opc_long as I did was so that 
for a 32-bit displacement like 0x12345678 instead of

      ldah $28,0x1234($31)
      lda  $28,0x5678($28)
      addq $16,$28,$28
      ldq  $17,0($28)

we can generate

      ldah $28,0x1234($16)
      ldq  $17,0x5678($28)

For larger 64-bit constants obviously we have no choice but to use an 
ADDQ, but even then one can stuff the final 16-bit addition into the 
memory insn itself.

Given the size of the CPUState, offsets > 32k are very common, so it 
would be good to handle this case well.

> +static void tcg_out_mov_addr( TCGContext *s, int ret, int addr)
> +{
> +    tcg_out_mov(s, ret, addr);
> +#if TARGET_LONG_BITS == 32
> +    /* if VM is of 32-bit arch, clear higher 32-bit of addr */
> +    tcg_out_fmt_opi(s, INSN_ZAPNOT, ret, 0x0f, ret);
> +#endif
> +}

It's a bit wasteful to emit the move *and* the zap; give "addr" to the 
first operand of the zapnot directly.

> +    tgen_arithi(s, INSN_ADDQ, r1, offsetof(CPUState, tlb_table[mem_index][0].addr_read));
> +    tcg_out_fmt_opr(s, INSN_ADDQ, r1, TCG_REG_15, r1);
> +#if TARGET_LONG_BITS == 32
> +    tcg_out_fmt_mem(s, INSN_LDL, TMP_REG1, r1, 0);
> +    tcg_out_fmt_opi(s, INSN_ZAPNOT, TMP_REG1, 0x0f, TMP_REG1);
> +#else
> +    tcg_out_fmt_mem(s, INSN_LDQ, TMP_REG1, r1, 0);
> +#endif

Better as

     tcg_out_fmt_opr(s, INSN_ADDQ, r1, TCG_REG_15, r1);
     tcg_out_ldst(s, TARGET_LONG_BITS == 32 ? INSN_LDL : INSN_LDQ,
                  TMP_REG1, r1, offsetof(CPUState, ...), 0);

to fold part of the offset into the memory displacement, and reuse your 
existing code emitting the zapnot.

You might also consider copying the ppc64 port and split out a 
tcg_out_tlb_read function from both qemu_ld and qemu_st.

> +    *(uint32_t *)label1_ptr = (uint32_t) \
> +        ( INSN_BNE | ( TMP_REG1 << 21 ) | ( val & 0x1fffff));

If you're not going to use the gen_new_label method, at least use your 
INSN_RA and INSN_DISP21 macros.

> +    tcg_out_movi(s, TCG_TYPE_I64, TMP_REG1, \
> +	offsetof(CPUTLBEntry, addend) - offsetof(CPUTLBEntry, addr_read));
> +    tcg_out_fmt_opr(s, INSN_ADDQ, r1, TMP_REG1, r1);
> +    tcg_out_fmt_mem(s, INSN_LDQ, TMP_REG1, r1, 0);

As above, better to use tcg_out_ldst.

> +    { INDEX_op_add_i32, { "r", "0", "r" } },

All of these matching constraints are wrong -- alpha is a 3 address 
machine; should be { "r", "r", "r" }, at least until you support 
constant arguments properly.

I still haven't seen anything that looks actively wrong to explain why 
windows isn't working for you...


r~