Re: [Qemu-devel] [PATCH 13/14] tcg-sparc: Fix 32-bit constant arguments tests

[Stuart Brady <sdb@zubnet.me.uk>]

On Mon, Mar 17, 2014 at 11:37:55AM -0700, Richard Henderson wrote:
> We need to discard garbage high bits before testing
> for 32-bit I and J constraints.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/sparc/tcg-target.c | 73 +++++++++++++++++++++++++++++---------------------
>  tcg/sparc/tcg-target.h |  4 ---
>  2 files changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/tcg/sparc/tcg-target.c b/tcg/sparc/tcg-target.c
> index 6498999..29d7a48 100644
> --- a/tcg/sparc/tcg-target.c
> +++ b/tcg/sparc/tcg-target.c
> @@ -89,6 +89,11 @@ static const char * const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
>  # define TCG_GUEST_BASE_REG TCG_REG_G0
>  #endif
>  
> +#define TCG_CT_CONST_S11  0x100
> +#define TCG_CT_CONST_S13  0x200
> +#define TCG_CT_CONST_ZERO 0x400
> +#define TCG_CT_CONST_IS32 0x800
> +
>  static const int tcg_target_reg_alloc_order[] = {
>      TCG_REG_L0,
>      TCG_REG_L1,
> @@ -357,6 +362,9 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str)
>      case 'Z':
>          ct->ct |= TCG_CT_CONST_ZERO;
>          break;
> +    case 'w':
> +        ct->ct |= TCG_CT_CONST_IS32;
> +        break;

It might be neater just to expose ts->type to tcg_target_const_match(),
which the TCG target can just ignore if it wants, but that might not
belong in this series, as this seems to affect PPC64 and IA64 too.

>      default:
>          return -1;
>      }
> @@ -373,7 +381,12 @@ static inline int tcg_target_const_match(tcg_target_long val,
>  
>      if (ct & TCG_CT_CONST) {
>          return 1;
> -    } else if ((ct & TCG_CT_CONST_ZERO) && val == 0) {
> +    }
> +    if (ct & TCG_CT_CONST_IS32) {
> +        val = (int32_t)val;
> +    }
> +
> +    if ((ct & TCG_CT_CONST_ZERO) && val == 0) {
>          return 1;
>      } else if ((ct & TCG_CT_CONST_S11) && check_fit_tl(val, 11)) {
>          return 1;
> @@ -679,7 +692,7 @@ static void tcg_out_setcond_i32(TCGContext *s, TCGCond cond, TCGReg ret,
>             swap the operands on GTU/LEU.  There's no benefit to loading
>             the constant into a temporary register.  */
>          if (!c2const || c2 == 0) {
> -            TCGArg t = c1;
> +            TCGReg t = c1;

I guess this was meant for 11/14?

>              c1 = c2;
>              c2 = t;
>              c2const = 0;
> @@ -1391,32 +1404,32 @@ static const TCGTargetOpDef sparc_op_defs[] = {
>      { INDEX_op_st16_i32, { "rZ", "r" } },
>      { INDEX_op_st_i32, { "rZ", "r" } },
>  
> -    { INDEX_op_add_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_mul_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_div_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_divu_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_sub_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_and_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_andc_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_or_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_orc_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_xor_i32, { "r", "rZ", "rJ" } },
> -
> -    { INDEX_op_shl_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_shr_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_sar_i32, { "r", "rZ", "rJ" } },
> -
> -    { INDEX_op_neg_i32, { "r", "rJ" } },
> -    { INDEX_op_not_i32, { "r", "rJ" } },
> -
> -    { INDEX_op_brcond_i32, { "rZ", "rJ" } },
> -    { INDEX_op_setcond_i32, { "r", "rZ", "rJ" } },
> -    { INDEX_op_movcond_i32, { "r", "rZ", "rJ", "rI", "0" } },
> -
> -    { INDEX_op_add2_i32, { "r", "r", "rZ", "rZ", "rJ", "rJ" } },
> -    { INDEX_op_sub2_i32, { "r", "r", "rZ", "rZ", "rJ", "rJ" } },
> -    { INDEX_op_mulu2_i32, { "r", "r", "rZ", "rJ" } },
> -    { INDEX_op_muls2_i32, { "r", "r", "rZ", "rJ" } },
> +    { INDEX_op_add_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_mul_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_div_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_divu_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_sub_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_and_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_andc_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_or_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_orc_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_xor_i32, { "r", "rZ", "rwJ" } },
> +
> +    { INDEX_op_shl_i32, { "r", "rZ", "ri" } },
> +    { INDEX_op_shr_i32, { "r", "rZ", "ri" } },
> +    { INDEX_op_sar_i32, { "r", "rZ", "ri" } },

Relaxation of the shift argument for shl/shr/sar seems fine, but it's not
mentioned in the description.

> +
> +    { INDEX_op_neg_i32, { "r", "r" } },
> +    { INDEX_op_not_i32, { "r", "r" } },

Right, constant folding deals with these... but likewise.

> +
> +    { INDEX_op_brcond_i32, { "rZ", "rwJ" } },
> +    { INDEX_op_setcond_i32, { "r", "rZ", "rwJ" } },
> +    { INDEX_op_movcond_i32, { "r", "rZ", "rwJ", "rwI", "0" } },
> +
> +    { INDEX_op_add2_i32, { "r", "r", "rZ", "rZ", "rwJ", "rwJ" } },
> +    { INDEX_op_sub2_i32, { "r", "r", "rZ", "rZ", "rwJ", "rwJ" } },
> +    { INDEX_op_mulu2_i32, { "r", "r", "rZ", "rwJ" } },
> +    { INDEX_op_muls2_i32, { "r", "r", "rZ", "rwJ" } },
>  
>      { INDEX_op_mov_i64, { "R", "R" } },
>      { INDEX_op_movi_i64, { "R" } },
> @@ -1447,8 +1460,8 @@ static const TCGTargetOpDef sparc_op_defs[] = {
>      { INDEX_op_shr_i64, { "R", "RZ", "RJ" } },
>      { INDEX_op_sar_i64, { "R", "RZ", "RJ" } },

As above.

>  
> -    { INDEX_op_neg_i64, { "R", "RJ" } },
> -    { INDEX_op_not_i64, { "R", "RJ" } },
> +    { INDEX_op_neg_i64, { "R", "R" } },
> +    { INDEX_op_not_i64, { "R", "R" } },

As above.

>  
>      { INDEX_op_ext32s_i64, { "R", "r" } },
>      { INDEX_op_ext32u_i64, { "R", "r" } },
> diff --git a/tcg/sparc/tcg-target.h b/tcg/sparc/tcg-target.h
> index 091224c..47fce69 100644
> --- a/tcg/sparc/tcg-target.h
> +++ b/tcg/sparc/tcg-target.h
> @@ -65,10 +65,6 @@ typedef enum {
>      TCG_REG_I7,
>  } TCGReg;
>  
> -#define TCG_CT_CONST_S11  0x100
> -#define TCG_CT_CONST_S13  0x200
> -#define TCG_CT_CONST_ZERO 0x400
> -
>  /* used for function call generation */
>  #define TCG_REG_CALL_STACK TCG_REG_O6
>  
> -- 
> 1.8.5.3
> 
> 
>  

Cheers,
Stuart