Questions/comments on TCG

Qemu-devel New Stuart Brady 2 participants 8 messages Mar 7, 2008 – Mar 8, 2008

Share & tools

Stuart Brady

author

2008-03-07 12:37 UTC #0 lore raw

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.

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.

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).

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.

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?

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.

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.

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

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.

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?

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.

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.

Cheers,

Signature · 1 line

Stuart Brady

Blue Swirl

2008-03-07 16:07 UTC #1 lore raw

Quoted 8 lines · Stuart Brady

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.

Quoted 3 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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

Right.

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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?

Quoted 2 lines

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.

Stuart Brady

author

2008-03-07 18:19 UTC #2 lore raw

Quoted 7 lines · Blue Swirl

Quoted 3 lines · Stuart Brady
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.

Don't worry -- I still checked with the x86 targets. I only really needed a quick idea of what was required for a new target.

Quoted 7 lines

Quoted 3 lines
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.

Sounds reasonable. I think I really meant to ask what _shouldn't_ go in tcg_target_reg_alloc_order[]. I was mainly confused by the inclusion of registers which are marked as reserved on the x86 targets.

Quoted 6 lines

Quoted 4 lines
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?

tcg_target_reg_alloc_order[] has 32 elements, but only 14 are used. The rest hold 0, specifying TCG_REG_G0.

Quoted 8 lines

Quoted 5 lines
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.

I don't understand -- o7 is required when returning in exit_tb, so if it is used, it must be saved and restored.

Quoted 6 lines

Quoted 3 lines
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.

Oh dear, I'm such a vim user, I don't even have Emacs installed. :)

How about something like this?

#if defined(__sparc_v9__) && !defined(__sparc_v8plus__)

#define OP_32_64(x)                             \
        glue(glue(case INDEX_op_, x), _i32:)    \
        glue(glue(case INDEX_op_, x), _i64)
#else
#define OP_32_64(x)                             \
        glue(glue(case INDEX_op_, x), _i32)
#endif
...
    OP_32_64(ld8u):
        tcg_out_ldst(s, args[0], args[1], args[2], LDUB);
        break;

...

The macro might be a bit sick, but hopefully it would make Emacs happy, and I feel ':' does make a certain amount of sense, here.

It probably wouldn't help with indentation, but you could always do something like this:

#if defined(__sparc_v9__) && !defined(__sparc_v8plus__) #define v9(x) x
#else
#define v9(x)
#endif
...

    case INDEX_op_ld8u_i32:
v9( case INDEX_op_ld8u_i64: )
        tcg_out_ldst(s, args[0], args[1], args[2], LDUB);
        break;

...

I'll admit, that looks unusual, but it would avoid breaking searches for ld8u_i32 or op_ld8u.

Cheers,

Signature · 1 line

Stuart Brady

Blue Swirl

2008-03-07 18:47 UTC #3 lore raw

Quoted 35 lines · Stuart Brady

Quoted 9 lines · Blue Swirl
Quoted 4 lines · Stuart Brady
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.
Don't worry -- I still checked with the x86 targets. I only really
needed a quick idea of what was required for a new target.
Quoted 7 lines
Quoted 3 lines
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.
Sounds reasonable. I think I really meant to ask what _shouldn't_ go in
tcg_target_reg_alloc_order[]. I was mainly confused by the inclusion of registers which are marked as reserved on the x86 targets.
Quoted 5 lines
Quoted 4 lines
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).

Quoted 5 lines

Quoted 1 line
Maybe I missed something, but g0 isn't in the reg_alloc_order?
tcg_target_reg_alloc_order[] has 32 elements, but only 14 are used.
The rest hold 0, specifying TCG_REG_G0.

I see. That could be asking for trouble.

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
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.

Quoted 6 lines

Quoted 2 lines
These are bugs, thanks for spotting. I was using o7 if a register is needed, it will be clobbered anyway.
I don't understand -- o7 is required when returning in exit_tb, so if it
is used, it must be saved and restored.

Not exit_tb, but call.

Quoted 24 lines

Quoted 6 lines
Quoted 3 lines
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.
Oh dear, I'm such a vim user, I don't even have Emacs installed. :)
How about something like this?
#if defined(__sparc_v9__) && !defined(__sparc_v8plus__)
 #define OP_32_64(x)                             \
        glue(glue(case INDEX_op_, x), _i32:)    \
        glue(glue(case INDEX_op_, x), _i64)
 #else
 #define OP_32_64(x)                             \
        glue(glue(case INDEX_op_, x), _i32)
 #endif
 ...
    OP_32_64(ld8u):
        tcg_out_ldst(s, args[0], args[1], args[2], LDUB);
        break;

Quoted 4 lines

...
The macro might be a bit sick, but hopefully it would make Emacs happy, and I feel ':' does make a certain amount of sense, here.

Unfortunately it confuses Emacs even more.

Quoted 9 lines

It probably wouldn't help with indentation, but you could always do something like this:
#if defined(__sparc_v9__) && !defined(__sparc_v8plus__) #define v9(x) x
#else
#define v9(x)
#endif
...

Quoted 8 lines

    case INDEX_op_ld8u_i32:
 v9( case INDEX_op_ld8u_i64: )
        tcg_out_ldst(s, args[0], args[1], args[2], LDUB);
        break;
...
I'll admit, that looks unusual, but it would avoid breaking searches for ld8u_i32 or op_ld8u.

This also does not work.

Stuart Brady

author

2008-03-07 19:56 UTC #4 lore raw

Quoted 5 lines · Blue Swirl

Quoted 2 lines · Stuart Brady
tcg_target_reg_alloc_order[] has 32 elements, but only 14 are used.
The rest hold 0, specifying TCG_REG_G0.
I see. That could be asking for trouble.

Possibly not, as g0 is marked as reserved, but it looks to me like bug, regardless of whether it causes any harm, so I've submitted a patch.

Quoted 4 lines

Quoted 2 lines
I don't understand -- o7 is required when returning in exit_tb, so if it
is used, it must be saved and restored.
Not exit_tb, but call.

Right, op_call does need to link, and that clobbers the link register, so it must be restored -- but I've a feeling that this isn't happening. I expect you could copy o7 to/from i5 before/after the call (or jmpl)... although I'm not sure if you'd also need to save the frame pointer.

op_exit_tb also needs the link register, as it needs to know where to return to. I see you've fixed op_goto_tb -- I'm not sure if TCG_REG_I5 now needs removing from tcg_target_reg_alloc_order[], though.

Cheers,

Signature · 1 line

Stuart Brady

Blue Swirl

2008-03-07 20:31 UTC #5 lore raw

Quoted 16 lines · Stuart Brady

Quoted 6 lines · Blue Swirl
Quoted 2 lines · Stuart Brady
tcg_target_reg_alloc_order[] has 32 elements, but only 14 are used.
The rest hold 0, specifying TCG_REG_G0.
I see. That could be asking for trouble.
Possibly not, as g0 is marked as reserved, but it looks to me like bug,
regardless of whether it causes any harm, so I've submitted a patch.
Quoted 3 lines
Quoted 2 lines
I don't understand -- o7 is required when returning in exit_tb, so if it
is used, it must be saved and restored.

Quoted 7 lines

Quoted 1 line
Not exit_tb, but call.
Right, op_call does need to link, and that clobbers the link register,
so it must be restored -- but I've a feeling that this isn't happening. I expect you could copy o7 to/from i5 before/after the call (or jmpl)... although I'm not sure if you'd also need to save the frame pointer.

Another possibility is to add function epilogue with save and add restore to ret (or use v9 return).

Quoted 3 lines

op_exit_tb also needs the link register, as it needs to know where to return to. I see you've fixed op_goto_tb -- I'm not sure if TCG_REG_I5 now needs removing from tcg_target_reg_alloc_order[], though.

Good point, better remove it.

Blue Swirl

2008-03-08 14:07 UTC #6 lore raw

Quoted 20 lines · Blue Swirl

Quoted 19 lines · Stuart Brady
Quoted 6 lines · Blue Swirl
Quoted 2 lines · Stuart Brady
tcg_target_reg_alloc_order[] has 32 elements, but only 14 are used.
The rest hold 0, specifying TCG_REG_G0.
I see. That could be asking for trouble.
Possibly not, as g0 is marked as reserved, but it looks to me like bug,
regardless of whether it causes any harm, so I've submitted a patch.
Quoted 4 lines
Quoted 2 lines
I don't understand -- o7 is required when returning in exit_tb, so if it
is used, it must be saved and restored.
Not exit_tb, but call.

Quoted 8 lines

Quoted 4 lines
Right, op_call does need to link, and that clobbers the link register,
so it must be restored -- but I've a feeling that this isn't happening. I expect you could copy o7 to/from i5 before/after the call (or jmpl)... although I'm not sure if you'd also need to save the frame pointer.
Another possibility is to add function epilogue with save and add
restore to ret (or use v9 return).

I added the save and restore instructions, because if the generated code made any calls, the registers were overwritten.

Currently on Sparc64 host a small helloworld program executes until the system call, then Qemu dies with illegal instruction. It looks like this is caused by setjmp/longjmp register mangling bugs in Linux glibc, my workaround does not help. I'd be interested to hear if this works any better on Solaris/Sparc or *BSD/Sparc. On Sparc32 TB linking does not work, so Qemu dies on TB switch.

#define __KERNEL__
#include <asm/unistd.h>
static int errno;
static __inline__ _syscall1(void,exit,int,exitval)
static inline _syscall3(int,write,int,fd,const char *,buf,long,count)

int _start()
{
write(2, "Hello World!\n", sizeof("Hello World!\n")); exit(0);
}

Stuart Brady

author

2008-03-07 18:28 UTC #7 lore raw

PATCH 1/1 Remove blank elements in tcg_target_reg_alloc_order[]

In reply to Stuart Brady

Hi,

tcg_target_reg_alloc_order[] contains blank elements, which default to TCG_REG_EAX/TCG_REG_RAX on i386/x86_64, and TCG_REG_G0 on SPARC. The included patch removes these elements, and adds an ARRAY_SIZE macro to osdep.h, which is then used to check the size of the array.

I'm not sure if the addition of ARRAY_SIZE is at all controversial. I'll happily resubmit the patch with this removed if it's disliked.

Cheers,

Signature · 72 lines

Stuart Brady

diff -urp qemu-orig/osdep.h qemu-new/osdep.h
--- qemu-orig/osdep.h	2008-01-31 19:42:53.000000000 +0000
+++ qemu-new/osdep.h	2008-03-07 15:44:49.000000000 +0000
@@ -26,6 +26,10 @@
 #define MAX(a, b) (((a) > (b)) ? (a) : (b))
 #endif
 
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
 #ifndef always_inline
 #if (__GNUC__ < 3) || defined(__APPLE__)
 #define always_inline inline
diff -urp qemu-orig/tcg/i386/tcg-target.c qemu-new/tcg/i386/tcg-target.c
--- qemu-orig/tcg/i386/tcg-target.c	2008-02-10 16:50:28.000000000 +0000
+++ qemu-new/tcg/i386/tcg-target.c	2008-03-07 15:26:47.000000000 +0000
@@ -32,7 +32,7 @@ const char *tcg_target_reg_names[TCG_TAR
     "%edi",
 };
 
-int tcg_target_reg_alloc_order[TCG_TARGET_NB_REGS] = {
+int tcg_target_reg_alloc_order[] = {
     TCG_REG_EAX,
     TCG_REG_EDX,
     TCG_REG_ECX,
diff -urp qemu-orig/tcg/sparc/tcg-target.c qemu-new/tcg/sparc/tcg-target.c
--- qemu-orig/tcg/sparc/tcg-target.c	2008-03-02 15:09:47.000000000 +0000
+++ qemu-new/tcg/sparc/tcg-target.c	2008-03-07 15:27:29.000000000 +0000
@@ -57,7 +57,7 @@ static const char * const tcg_target_reg
     "%i7",
 };
 
-static const int tcg_target_reg_alloc_order[TCG_TARGET_NB_REGS] = {
+static const int tcg_target_reg_alloc_order[] = {
     TCG_REG_L0,
     TCG_REG_L1,
     TCG_REG_L2,
diff -urp qemu-orig/tcg/tcg.c qemu-new/tcg/tcg.c
--- qemu-orig/tcg/tcg.c	2008-02-19 01:21:18.000000000 +0000
+++ qemu-new/tcg/tcg.c	2008-03-07 15:26:23.000000000 +0000
@@ -1212,14 +1212,14 @@ static int tcg_reg_alloc(TCGContext *s, 
     tcg_regset_andnot(reg_ct, reg1, reg2);
 
     /* first try free registers */
-    for(i = 0; i < TCG_TARGET_NB_REGS; i++) {
+    for(i = 0; i < ARRAY_SIZE(tcg_target_reg_alloc_order); i++) {
         reg = tcg_target_reg_alloc_order[i];
         if (tcg_regset_test_reg(reg_ct, reg) && s->reg_to_temp[reg] == -1)
             return reg;
     }
 
     /* XXX: do better spill choice */
-    for(i = 0; i < TCG_TARGET_NB_REGS; i++) {
+    for(i = 0; i < ARRAY_SIZE(tcg_target_reg_alloc_order); i++) {
         reg = tcg_target_reg_alloc_order[i];
         if (tcg_regset_test_reg(reg_ct, reg)) {
             tcg_reg_free(s, reg);
diff -urp qemu-orig/tcg/x86_64/tcg-target.c qemu-new/tcg/x86_64/tcg-target.c
--- qemu-orig/tcg/x86_64/tcg-target.c	2008-03-02 15:09:47.000000000 +0000
+++ qemu-new/tcg/x86_64/tcg-target.c	2008-03-07 15:27:14.000000000 +0000
@@ -40,7 +40,7 @@ const char *tcg_target_reg_names[TCG_TAR
     "%r15",
 };
 
-int tcg_target_reg_alloc_order[TCG_TARGET_NB_REGS] = {
+int tcg_target_reg_alloc_order[] = {
     TCG_REG_RDI,
     TCG_REG_RSI,
     TCG_REG_RDX,