Re: [Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.
On 06/10/2010 03:22 AM, Aurelien Jarno wrote:
>> - 0, GET_TCGV_I32(ret), 2, args);
>> + (is_signed ? 0x2a : 0x00), GET_TCGV_I32(ret), 2, args);
>
> Wouldn't it be better to actually pass the whole flag to
> tcg_gen_helper32(), so that we can in the future also support mixed
> signedness in arguments? Also doing it here looks like a bit like a
> magic constant.
I've fixed this.
>> +#if defined(TCG_TARGET_EXTEND_ARGS) && TCG_TARGET_REG_BITS == 64
>> +for (i = 0; i < nargs; ++i) {
>> +int is_64bit = sizemask & (1 << (i+1)*2);
>> +int is_signed = sizemask & (2 << (i+1)*2);
>> +if (!is_64bit) {
>> +TCGv_i64 temp = tcg_temp_new_i64();
>> +TCGv_i64 orig = MAKE_TCGV_I64(args[i]);
>> +if (is_signed) {
>> +tcg_gen_ext32s_i64(temp, orig);
>> +} else {
>> +tcg_gen_ext32u_i64(temp, orig);
>> +}
>> +args[i] = GET_TCGV_I64(temp);
>> +}
>> +}
>> +#endif /* TCG_TARGET_EXTEND_ARGS */
>> +
>
> This part allocates a lot of temp variables, that will probably generate
> a lot of register spills during the code generation.
>
> As we do that for all arguments anyway, wouldn't it be possible to do
> the extension in place? The value in the register is changed, but that
> should not have any effect as it is ignored anyway in other
> instructions.
It is *not* possible to do the extension in-place. At least not without
changing the format of the INDEX_op_call opcode.
With the extension done during opcode generation, like this, ORIG has been
marked TCG_TYPE_I32 and TEMP gets marked TCG_TYPE_I64. This matters when
it comes time to copy the arguments into place. If we somehow extended
ORIG in-place, we'd still use the wrong instruction to copy the value into
the argument list -- either tcg_out_mov or tcg_out_st would get the wrong
TYPE argument.
If we try to do the extension later, e.g. while copying the value into the
argument list, we'd need to have SIZEMASK available. To do that, we'd need
to save SIZEMASK into INDEX_op_call's argument list somehow. That, I think,
is a more invasive change.
r~
Re: [Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.
On 06/10/2010 03:22 AM, Aurelien Jarno wrote: > Wouldn't it be better to actually pass the whole flag to > tcg_gen_helper32(), so that we can in the future also support mixed > signedness in arguments? Also doing it here looks like a bit like a > magic constant. I suppose that's possible. > This part allocates a lot of temp variables, that will probably generate > a lot of register spills during the code generation. > > As we do that for all arguments anyway, wouldn't it be possible to do > the extension in place? The value in the register is changed, but that > should not have any effect as it is ignored anyway in other > instructions. That hadn't occurred to me. I'll give it a try. r~
Re: [Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.
On Fri, Jun 04, 2010 at 12:14:15PM -0700, Richard Henderson wrote:
> Some hosts (amd64, ia64) have an ABI that ignores the high bits
> of the 64-bit register when passing 32-bit arguments. Others,
> like s390x, require the value to be properly sign-extended for
> the type. I.e. "int32_t" must be sign-extended and "uint32_t"
> must be zero-extended to 64-bits.
>
> To effect this, extend the "sizemask" parameter to tcg_gen_callN
> to include the signedness of the type of each parameter. If the
> tcg target requires it, extend each 32-bit argument into a 64-bit
> temp and pass that to the function call.
>
> Signed-off-by: Richard Henderson
> ---
> def-helper.h | 38 +-
> target-i386/ops_sse_header.h |3 +++
> target-ppc/helper.h |1 +
> tcg/s390/tcg-target.h|2 ++
> tcg/tcg-op.h | 42
> +-
> tcg/tcg.c| 41 +++--
> 6 files changed, 91 insertions(+), 36 deletions(-)
>
> diff --git a/def-helper.h b/def-helper.h
> index 8a88c5b..8a822c7 100644
> --- a/def-helper.h
> +++ b/def-helper.h
> @@ -81,9 +81,29 @@
> #define dh_is_64bit_ptr (TCG_TARGET_REG_BITS == 64)
> #define dh_is_64bit(t) glue(dh_is_64bit_, dh_alias(t))
>
> +#define dh_is_signed_void 0
> +#define dh_is_signed_i32 0
> +#define dh_is_signed_s32 1
> +#define dh_is_signed_i64 0
> +#define dh_is_signed_s64 1
> +#define dh_is_signed_f32 0
> +#define dh_is_signed_f64 0
> +#define dh_is_signed_tl 0
> +#define dh_is_signed_int 1
> +/* ??? This is highly specific to the host cpu. There are even special
> + extension instructions that may be required, e.g. ia64's addp4. But
> + for now we don't support any 64-bit targets with 32-bit pointers. */
> +#define dh_is_signed_ptr 0
> +#define dh_is_signed_env dh_is_signed_ptr
> +#define dh_is_signed(t) dh_is_signed_##t
> +
> +#define dh_sizemask(t, n) \
> + sizemask |= dh_is_64bit(t) << (n*2); \
> + sizemask |= dh_is_signed(t) << (n*2+1)
> +
> #define dh_arg(t, n) \
>args[n - 1] = glue(GET_TCGV_, dh_alias(t))(glue(arg, n)); \
> - sizemask |= dh_is_64bit(t) << n
> + dh_sizemask(t, n)
>
> #define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
>
> @@ -138,8 +158,8 @@ static inline void glue(gen_helper_,
> name)(dh_retvar_decl0(ret)) \
> static inline void glue(gen_helper_, name)(dh_retvar_decl(ret)
> dh_arg_decl(t1, 1)) \
> { \
>TCGArg args[1]; \
> - int sizemask; \
> - sizemask = dh_is_64bit(ret); \
> + int sizemask = 0; \
> + dh_sizemask(ret, 0); \
>dh_arg(t1, 1); \
>tcg_gen_helperN(HELPER(name), flags, sizemask, dh_retvar(ret), 1, args); \
> }
> @@ -149,8 +169,8 @@ static inline void glue(gen_helper_,
> name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
> dh_arg_decl(t2, 2)) \
> { \
>TCGArg args[2]; \
> - int sizemask; \
> - sizemask = dh_is_64bit(ret); \
> + int sizemask = 0; \
> + dh_sizemask(ret, 0); \
>dh_arg(t1, 1); \
>dh_arg(t2, 2); \
>tcg_gen_helperN(HELPER(name), flags, sizemask, dh_retvar(ret), 2, args); \
> @@ -161,8 +181,8 @@ static inline void glue(gen_helper_,
> name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
> dh_arg_decl(t2, 2), dh_arg_decl(t3, 3)) \
> { \
>TCGArg args[3]; \
> - int sizemask; \
> - sizemask = dh_is_64bit(ret); \
> + int sizemask = 0; \
> + dh_sizemask(ret, 0); \
>dh_arg(t1, 1); \
>dh_arg(t2, 2); \
>dh_arg(t3, 3); \
> @@ -174,8 +194,8 @@ static inline void glue(gen_helper_,
> name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
> dh_arg_decl(t2, 2), dh_arg_decl(t3, 3), dh_arg_decl(t4, 4)) \
> { \
>TCGArg args[4]; \
> - int sizemask; \
> - sizemask = dh_is_64bit(ret); \
> + int sizemask = 0; \
> + dh_sizemask(ret, 0); \
>dh_arg(t1, 1); \
>dh_arg(t2, 2); \
>dh_arg(t3, 3); \
> diff --git a/target-i386/ops_sse_header.h b/target-i386/ops_sse_header.h
> index a0a6361..8d4b2b7 100644
> --- a/target-i386/ops_sse_header.h
> +++ b/target-i386/ops_sse_header.h
> @@ -30,6 +30,9 @@
> #define dh_ctype_Reg Reg *
> #define dh_ctype_XMMReg XMMReg *
> #define dh_ctype_MMXReg MMXReg *
> +#define dh_is_signed_Reg dh_is_signed_ptr
> +#define dh_is_signed_XMMReg dh_is_signed_ptr
> +#define dh_is_signed_MMXReg dh_is_signed_ptr
>
> DEF_HELPER_2(glue(psrlw, SUFFIX), void, Reg, Reg)
> DEF_HELPER_2(glue(psraw, SUFFIX), void, Reg, Reg)
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 5cf6cd4..c025a2f 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -95,6 +95,7 @@ DEF_HELPER_3(fsel, i64, i64, i64, i64)
>
> #define dh_alias_avr ptr
> #define dh_ctype_avr ppc_avr_t *
> +#define dh_is_signed_avr dh_is_signed_ptr
>
> DEF_HELPER_3(vaddubm, void, avr, avr, avr)
> DEF_HELPER_3(vadduhm, void, avr, avr, avr)
> diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h
> index d7fe0c7..8c19262 100644
> --- a/tcg/s390/tcg-target.h
> +++ b/tcg/s390/tcg-target.h
[Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.
Some hosts (amd64, ia64) have an ABI that ignores the high bits
of the 64-bit register when passing 32-bit arguments. Others,
like s390x, require the value to be properly sign-extended for
the type. I.e. "int32_t" must be sign-extended and "uint32_t"
must be zero-extended to 64-bits.
To effect this, extend the "sizemask" parameter to tcg_gen_callN
to include the signedness of the type of each parameter. If the
tcg target requires it, extend each 32-bit argument into a 64-bit
temp and pass that to the function call.
Signed-off-by: Richard Henderson
---
def-helper.h | 38 +-
target-i386/ops_sse_header.h |3 +++
target-ppc/helper.h |1 +
tcg/s390/tcg-target.h|2 ++
tcg/tcg-op.h | 42 +-
tcg/tcg.c| 41 +++--
6 files changed, 91 insertions(+), 36 deletions(-)
diff --git a/def-helper.h b/def-helper.h
index 8a88c5b..8a822c7 100644
--- a/def-helper.h
+++ b/def-helper.h
@@ -81,9 +81,29 @@
#define dh_is_64bit_ptr (TCG_TARGET_REG_BITS == 64)
#define dh_is_64bit(t) glue(dh_is_64bit_, dh_alias(t))
+#define dh_is_signed_void 0
+#define dh_is_signed_i32 0
+#define dh_is_signed_s32 1
+#define dh_is_signed_i64 0
+#define dh_is_signed_s64 1
+#define dh_is_signed_f32 0
+#define dh_is_signed_f64 0
+#define dh_is_signed_tl 0
+#define dh_is_signed_int 1
+/* ??? This is highly specific to the host cpu. There are even special
+ extension instructions that may be required, e.g. ia64's addp4. But
+ for now we don't support any 64-bit targets with 32-bit pointers. */
+#define dh_is_signed_ptr 0
+#define dh_is_signed_env dh_is_signed_ptr
+#define dh_is_signed(t) dh_is_signed_##t
+
+#define dh_sizemask(t, n) \
+ sizemask |= dh_is_64bit(t) << (n*2); \
+ sizemask |= dh_is_signed(t) << (n*2+1)
+
#define dh_arg(t, n) \
args[n - 1] = glue(GET_TCGV_, dh_alias(t))(glue(arg, n)); \
- sizemask |= dh_is_64bit(t) << n
+ dh_sizemask(t, n)
#define dh_arg_decl(t, n) glue(TCGv_, dh_alias(t)) glue(arg, n)
@@ -138,8 +158,8 @@ static inline void glue(gen_helper_,
name)(dh_retvar_decl0(ret)) \
static inline void glue(gen_helper_, name)(dh_retvar_decl(ret) dh_arg_decl(t1,
1)) \
{ \
TCGArg args[1]; \
- int sizemask; \
- sizemask = dh_is_64bit(ret); \
+ int sizemask = 0; \
+ dh_sizemask(ret, 0); \
dh_arg(t1, 1); \
tcg_gen_helperN(HELPER(name), flags, sizemask, dh_retvar(ret), 1, args); \
}
@@ -149,8 +169,8 @@ static inline void glue(gen_helper_,
name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
dh_arg_decl(t2, 2)) \
{ \
TCGArg args[2]; \
- int sizemask; \
- sizemask = dh_is_64bit(ret); \
+ int sizemask = 0; \
+ dh_sizemask(ret, 0); \
dh_arg(t1, 1); \
dh_arg(t2, 2); \
tcg_gen_helperN(HELPER(name), flags, sizemask, dh_retvar(ret), 2, args); \
@@ -161,8 +181,8 @@ static inline void glue(gen_helper_,
name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
dh_arg_decl(t2, 2), dh_arg_decl(t3, 3)) \
{ \
TCGArg args[3]; \
- int sizemask; \
- sizemask = dh_is_64bit(ret); \
+ int sizemask = 0; \
+ dh_sizemask(ret, 0); \
dh_arg(t1, 1); \
dh_arg(t2, 2); \
dh_arg(t3, 3); \
@@ -174,8 +194,8 @@ static inline void glue(gen_helper_,
name)(dh_retvar_decl(ret) dh_arg_decl(t1, 1
dh_arg_decl(t2, 2), dh_arg_decl(t3, 3), dh_arg_decl(t4, 4)) \
{ \
TCGArg args[4]; \
- int sizemask; \
- sizemask = dh_is_64bit(ret); \
+ int sizemask = 0; \
+ dh_sizemask(ret, 0); \
dh_arg(t1, 1); \
dh_arg(t2, 2); \
dh_arg(t3, 3); \
diff --git a/target-i386/ops_sse_header.h b/target-i386/ops_sse_header.h
index a0a6361..8d4b2b7 100644
--- a/target-i386/ops_sse_header.h
+++ b/target-i386/ops_sse_header.h
@@ -30,6 +30,9 @@
#define dh_ctype_Reg Reg *
#define dh_ctype_XMMReg XMMReg *
#define dh_ctype_MMXReg MMXReg *
+#define dh_is_signed_Reg dh_is_signed_ptr
+#define dh_is_signed_XMMReg dh_is_signed_ptr
+#define dh_is_signed_MMXReg dh_is_signed_ptr
DEF_HELPER_2(glue(psrlw, SUFFIX), void, Reg, Reg)
DEF_HELPER_2(glue(psraw, SUFFIX), void, Reg, Reg)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 5cf6cd4..c025a2f 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -95,6 +95,7 @@ DEF_HELPER_3(fsel, i64, i64, i64, i64)
#define dh_alias_avr ptr
#define dh_ctype_avr ppc_avr_t *
+#define dh_is_signed_avr dh_is_signed_ptr
DEF_HELPER_3(vaddubm, void, avr, avr, avr)
DEF_HELPER_3(vadduhm, void, avr, avr, avr)
diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h
index d7fe0c7..8c19262 100644
--- a/tcg/s390/tcg-target.h
+++ b/tcg/s390/tcg-target.h
@@ -87,6 +87,8 @@ enum {
#define TCG_TARGET_STACK_ALIGN 8
#define TCG_TARGET_CALL_STACK_OFFSET 0
+#define TCG_TARGET_EXTEND_ARGS 1
+
enum {
/* Note: must be synced with dyngen-exec.h */
TCG_AREG0 = TCG_REG_R10,
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index aa436de..4220e3d 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-
