Re: [Qemu-devel] Questions/comments on TCG

[Blue Swirl]

On 3/7/08, Stuart Brady <address@hidden> wrote:
> Hi,
>
>  I have a few questions regarding TCG which I'd like to ask, and also a
>  few minor comments.  I've made a start on a PA-RISC (HPPA) TCG target,
>  but there are a few things that I'm not sure of.  Before I ask, I should
>  make it clear that I do understand that the current SPARC TCG code is
>  preliminary work -- however, in some ways, I feel it still serves as a
>  better reference than i386 and x86_64.

Well, I'd still recommend using x86 as a reference until Sparc works
or you may copy a faulty design.

>  Which registers should go in tcg_target_reg_alloc_order[]?  I notice
>  that i386 includes ESP, which tcg_target_init() marks as reserved, and
>  x86_64 includes RBX and RBP, which are again marked as reserved.

I put there only the registers that should be safe to use, the G
registers may have issues or they are already used as global
registers. Also we should not need frame pointer.

>  Furthermore, x86_64's tcg_target_reg_alloc_order[] contains 16 elements
>  (TCG_TARGET_NB_REGS), but only 15 are specified -- the last element is
>  left as 0, which is TCG_REG_RAX.  SPARC also does this, but with
>  TARGET_REG_G0 (which is marked as reserved, as it's hardwired to zero).

Maybe I missed something, but g0 isn't in the reg_alloc_order?

>  patch_reloc() takes a 'value' parameter with addend already added -- it
>  might be preferable for some architectures to pass addend separately, as
>  this would allow the relocations to be written in a manner which follows
>  the ABI specification more closely.  On HPPA, the definitions of the
>  relocations are so twisty, I'd like to avoid any possible thinkos here.
>  As patch_reloc() is static, this would not slow the other targets down.

I'm not sure about patch_reloc use. On Sparc there are other
unimplemented relocations that should be much more common, but TCG
never aborts in patch_reloc.

>  The value returned by tcg_target_get_call_iarg_regs_count() is fairly
>  unsurprisingly used as an upper limit for tcg_target_call_iarg_regs[].
>  Is there any reason that ARRAY_SIZE(tcg_target_call_iarg_regs) wasn't
>  used instead?

Good question. There are also other uses of arrays where NULL is used
as a terminator when the size would always be known anyway.

>  On SPARC, I notice that goto_tb is handled using CALL and JMPL, placing
>  the return address in o7... but we're returning from a TB, or jumping to
>  another one, so surely we shouldn't link here?  Also, TCG_TYPE_TL is
>  used for exit_tb's return value, I think this should be the host's long
>  (using TCG_TYPE_PTR) instead.

These are bugs, thanks for spotting. I was using o7 if a register is
needed, it will be clobbered anyway.

>  Also on SPARC, could the indentation of the OP_32_64s be improved?
>  Yeah, it's not a serious problem, but I feel it would make the code
>  slightly easier to read.

It's not my fault, Emacs wants to do it this way. I'm open to your suggestions.

>  Perhaps gen_arith would be better placed at the end of tcg_out_op()?

Right.

>  Another possible readability improvement -- the 'tcg_out_' prefix gives
>  the impression of being part of a defined interface, but target-specific
>  functions, such as tcg_out_modrm(), share this prefix.  This is made
>  worse by the fact that every function in tcg-target.c should be declared
>  static (although some of them aren't).
>
>  64-bit SPARC lists ld_i32u_i64 and st_i32_i64 -- the correct names of
>  these are ld32u_i64, ld32s_i64 and st32_i64.

Yes, I'll fix these.

>  Again on SPARC, for the arithmetic ops, the constraints used are
>  { "r", "0", "rJ" }.  If I've understood correctly, this would mean that
>  the output can go in any general purpose register, the first operand
>  must be that same register as the output, and the second operand must be
>  either another general purpose register, or a 13-bit signed immediate
>  constant.  In tcg_out_arith(), 'rd', 'r1' and 'r2' can each specifiy a
>  different register, so should this be { "r", "r", "rJ" }, instead?

Oh, that's what the '0' means. I'll try your version.

>  There is one register on PA-RISC that I'm not sure whether to reserve:
>  r19 is call-clobbered, unless position independent code is generated.
>  If I fail to list r19 in tcg_target_call_clobber_regs, it would be
>  corrupted when calling non-PIC helpers, but if I do list r19 as a call-
>  clobbered register, it would be used without being saved when PIC is
>  generated.  I expect that I can get away with assuming that PIC is not
>  used, but I'm wondering if it would be better to avoid that assumption.

Maybe tcg_target_init should take a flag parameter?

>  I will submit patches where I have some idea of what's required, but
>  I do need to be corrected if there's anything I've misunderstood.

I'm not sure if I have understood everything correctly either.