Re: [Qemu-devel] [PATCH 07/35] tcg: Optionally sign-extend 32-bit arguments for 64-bit host.

[Aurelien Jarno]

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 <address@hidden>
> ---
>  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-op.h
> @@ -369,8 +369,8 @@ static inline void tcg_gen_helperN(void *func, int flags, 
> int sizemask,
>     and pure, hence the call to tcg_gen_callN() with TCG_CALL_CONST |
>     TCG_CALL_PURE. This may need to be adjusted if these functions
>     start to be used with other helpers. */
> -static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
> -                                    TCGv_i32 a, TCGv_i32 b)
> +static inline void tcg_gen_helper32(void *func, TCGv_i32 ret, TCGv_i32 a,
> +                                    TCGv_i32 b, _Bool is_signed)

This should be int instead of _Bool.

>  {
>      TCGv_ptr fn;
>      TCGArg args[2];
> @@ -378,12 +378,12 @@ static inline void tcg_gen_helper32(void *func, 
> TCGv_i32 ret,
>      args[0] = GET_TCGV_I32(a);
>      args[1] = GET_TCGV_I32(b);
>      tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
> -                  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.

>      tcg_temp_free_ptr(fn);
>  }
>  
> -static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
> -                                    TCGv_i64 a, TCGv_i64 b)
> +static inline void tcg_gen_helper64(void *func, TCGv_i64 ret, TCGv_i64 a,
> +                                    TCGv_i64 b, _Bool is_signed)
>  {
>      TCGv_ptr fn;
>      TCGArg args[2];
> @@ -391,7 +391,7 @@ static inline void tcg_gen_helper64(void *func, TCGv_i64 
> ret,
>      args[0] = GET_TCGV_I64(a);
>      args[1] = GET_TCGV_I64(b);
>      tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
> -                  7, GET_TCGV_I64(ret), 2, args);
> +                  (is_signed ? 0x3f : 0x15), GET_TCGV_I64(ret), 2, args);
>      tcg_temp_free_ptr(fn);
>  }

Same

> @@ -692,22 +692,22 @@ static inline void tcg_gen_remu_i32(TCGv_i32 ret, 
> TCGv_i32 arg1, TCGv_i32 arg2)
>  #else
>  static inline void tcg_gen_div_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 
> arg2)
>  {
> -    tcg_gen_helper32(tcg_helper_div_i32, ret, arg1, arg2);
> +    tcg_gen_helper32(tcg_helper_div_i32, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_rem_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 
> arg2)
>  {
> -    tcg_gen_helper32(tcg_helper_rem_i32, ret, arg1, arg2);
> +    tcg_gen_helper32(tcg_helper_rem_i32, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_divu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 
> arg2)
>  {
> -    tcg_gen_helper32(tcg_helper_divu_i32, ret, arg1, arg2);
> +    tcg_gen_helper32(tcg_helper_divu_i32, ret, arg1, arg2, 0);
>  }
>  
>  static inline void tcg_gen_remu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 
> arg2)
>  {
> -    tcg_gen_helper32(tcg_helper_remu_i32, ret, arg1, arg2);
> +    tcg_gen_helper32(tcg_helper_remu_i32, ret, arg1, arg2, 0);
>  }
>  #endif
>  
> @@ -867,7 +867,7 @@ static inline void tcg_gen_xori_i64(TCGv_i64 ret, 
> TCGv_i64 arg1, int64_t arg2)
>     specific code (x86) */
>  static inline void tcg_gen_shl_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_shl_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_shl_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_shli_i64(TCGv_i64 ret, TCGv_i64 arg1, int64_t 
> arg2)
> @@ -877,7 +877,7 @@ static inline void tcg_gen_shli_i64(TCGv_i64 ret, 
> TCGv_i64 arg1, int64_t arg2)
>  
>  static inline void tcg_gen_shr_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_shr_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_shr_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_shri_i64(TCGv_i64 ret, TCGv_i64 arg1, int64_t 
> arg2)
> @@ -887,7 +887,7 @@ static inline void tcg_gen_shri_i64(TCGv_i64 ret, 
> TCGv_i64 arg1, int64_t arg2)
>  
>  static inline void tcg_gen_sar_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_sar_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_sar_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_sari_i64(TCGv_i64 ret, TCGv_i64 arg1, int64_t 
> arg2)
> @@ -935,22 +935,22 @@ static inline void tcg_gen_mul_i64(TCGv_i64 ret, 
> TCGv_i64 arg1, TCGv_i64 arg2)
>  
>  static inline void tcg_gen_div_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_div_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_div_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_rem_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_rem_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_rem_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_divu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_divu_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_divu_i64, ret, arg1, arg2, 0);
>  }
>  
>  static inline void tcg_gen_remu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_remu_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_remu_i64, ret, arg1, arg2, 0);
>  }
>  
>  #else
> @@ -1212,22 +1212,22 @@ static inline void tcg_gen_remu_i64(TCGv_i64 ret, 
> TCGv_i64 arg1, TCGv_i64 arg2)
>  #else
>  static inline void tcg_gen_div_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_div_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_div_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_rem_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_rem_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_rem_i64, ret, arg1, arg2, 1);
>  }
>  
>  static inline void tcg_gen_divu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_divu_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_divu_i64, ret, arg1, arg2, 0);
>  }
>  
>  static inline void tcg_gen_remu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 
> arg2)
>  {
> -    tcg_gen_helper64(tcg_helper_remu_i64, ret, arg1, arg2);
> +    tcg_gen_helper64(tcg_helper_remu_i64, ret, arg1, arg2, 0);
>  }
>  #endif
>  
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 880e7ce..d8ddd1f 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -560,6 +560,24 @@ void tcg_gen_callN(TCGContext *s, TCGv_ptr func, 
> unsigned int flags,
>      int real_args;
>      int nb_rets;
>      TCGArg *nparam;
> +
> +#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.

>      *gen_opc_ptr++ = INDEX_op_call;
>      nparam = gen_opparam_ptr++;
>  #ifdef TCG_TARGET_I386
> @@ -588,7 +606,8 @@ void tcg_gen_callN(TCGContext *s, TCGv_ptr func, unsigned 
> int flags,
>      real_args = 0;
>      for (i = 0; i < nargs; i++) {
>  #if TCG_TARGET_REG_BITS < 64
> -        if (sizemask & (2 << i)) {
> +        int is_64bit = sizemask & (1 << (i+1)*2);
> +        if (is_64bit) {
>  #ifdef TCG_TARGET_I386
>              /* REGPARM case: if the third parameter is 64 bit, it is
>                 allocated on the stack */
> @@ -622,12 +641,12 @@ void tcg_gen_callN(TCGContext *s, TCGv_ptr func, 
> unsigned int flags,
>              *gen_opparam_ptr++ = args[i] + 1;
>  #endif
>              real_args += 2;
> -        } else
> -#endif
> -        {
> -            *gen_opparam_ptr++ = args[i];
> -            real_args++;
> +            continue;
>          }
> +#endif /* TCG_TARGET_REG_BITS < 64 */
> +
> +        *gen_opparam_ptr++ = args[i];
> +        real_args++;
>      }
>      *gen_opparam_ptr++ = GET_TCGV_PTR(func);
>  
> @@ -637,6 +656,16 @@ void tcg_gen_callN(TCGContext *s, TCGv_ptr func, 
> unsigned int flags,
>  
>      /* total parameters, needed to go backward in the instruction stream */
>      *gen_opparam_ptr++ = 1 + nb_rets + real_args + 3;
> +
> +#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);
> +        if (!is_64bit) {
> +            TCGv_i64 temp = MAKE_TCGV_I64(args[i]);
> +            tcg_temp_free_i64(temp);
> +        }
> +    }
> +#endif /* TCG_TARGET_EXTEND_ARGS */
>  }
>  
>  #if TCG_TARGET_REG_BITS == 32
> -- 
> 1.7.0.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net