Re: [Qemu-devel] [PATCH v4 11/33] tcg-aarch64: Handle constant operands to add, sub, and compare

[Claudio Fontana]

On 14.09.2013 23:54, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/aarch64/tcg-target.c | 103 
> ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 80 insertions(+), 23 deletions(-)
> 
> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c
> index 93badfd..59499fd 100644
> --- a/tcg/aarch64/tcg-target.c
> +++ b/tcg/aarch64/tcg-target.c
> @@ -111,6 +111,9 @@ static inline void patch_reloc(uint8_t *code_ptr, int 
> type,
>      }
>  }
>  
> +#define TCG_CT_CONST_IS32 0x100
> +#define TCG_CT_CONST_AIMM 0x200
> +
>  /* parse target specific constraints */
>  static int target_parse_constraint(TCGArgConstraint *ct,
>                                     const char **pct_str)
> @@ -134,6 +137,12 @@ static int target_parse_constraint(TCGArgConstraint *ct,
>          tcg_regset_reset_reg(ct->u.regs, TCG_REG_X3);
>  #endif
>          break;
> +    case 'w': /* The operand should be considered 32-bit.  */
> +        ct->ct |= TCG_CT_CONST_IS32;
> +        break;
> +    case 'A': /* Valid for arithmetic immediate (positive or negative).  */
> +        ct->ct |= TCG_CT_CONST_AIMM;
> +        break;
>      default:
>          return -1;
>      }
> @@ -143,14 +152,25 @@ static int target_parse_constraint(TCGArgConstraint *ct,
>      return 0;
>  }
>  
> -static inline int tcg_target_const_match(tcg_target_long val,
> -                                         const TCGArgConstraint *arg_ct)
> +static inline bool is_aimm(uint64_t val)
> +{
> +    return (val & ~0xfff) == 0 || (val & ~0xfff000) == 0;
> +}
> +
> +static int tcg_target_const_match(tcg_target_long val,
> +                                  const TCGArgConstraint *arg_ct)
>  {
>      int ct = arg_ct->ct;
>  
>      if (ct & TCG_CT_CONST) {
>          return 1;
>      }
> +    if (ct & TCG_CT_CONST_IS32) {
> +        val = (int32_t)val;
> +    }
> +    if ((ct & TCG_CT_CONST_AIMM) && (is_aimm(val) || is_aimm(-val))) {
> +        return 1;
> +    }
>  
>      return 0;
>  }
> @@ -558,11 +578,21 @@ static inline void tcg_out_rotl(TCGContext *s, TCGType 
> ext,
>      tcg_out_extr(s, ext, rd, rn, rn, bits - (m & max));
>  }
>  
> -static inline void tcg_out_cmp(TCGContext *s, TCGType ext, TCGReg rn,
> -                               TCGReg rm)
> +static void tcg_out_cmp(TCGContext *s, TCGType ext, TCGReg a,
> +                        tcg_target_long b, bool const_b)
>  {
> -    /* Using CMP alias SUBS wzr, Wn, Wm */
> -    tcg_fmt_Rdnm(s, INSN_SUBS, ext, TCG_REG_XZR, rn, rm);
> +    if (const_b) {
> +        /* Using CMP or CMN aliases.  */
> +        AArch64Insn insn = INSN_SUBSI;
> +        if (b < 0) {
> +            insn = INSN_ADDSI;
> +            b = -b;
> +        }
> +        tcg_fmt_Rdn_aimm(s, insn, ext, TCG_REG_XZR, a, b);
> +    } else {
> +        /* Using CMP alias SUBS wzr, Wn, Wm */
> +        tcg_fmt_Rdnm(s, INSN_SUBS, ext, TCG_REG_XZR, a, b);
> +    }
>  }

What about splitting into tcg_out_cmp_imm and tcg_out_cmp_reg?
or tcg_out_cmp_i and tcg_out_cmp_r.
The function is an 'if else' anyway with no code sharing, and we would avoid 
sidestepping the TCGReg type check for b in the _r case, as well as the const_b 
additional param.

>  
>  static inline void tcg_out_cset(TCGContext *s, TCGType ext,
> @@ -760,6 +790,17 @@ static inline void tcg_out_uxt(TCGContext *s, int s_bits,
>      tcg_out_ubfm(s, 0, rd, rn, 0, bits);
>  }
>  
> +static void tcg_out_addsubi(TCGContext *s, int ext, TCGReg rd,
> +                            TCGReg rn, int aimm)
> +{
> +    AArch64Insn insn = INSN_ADDI;
> +    if (aimm < 0) {
> +        insn = INSN_SUBI;
> +        aimm = -aimm;
> +    }
> +    tcg_fmt_Rdn_aimm(s, insn, ext, rd, rn, aimm);
> +}
> +

Could this be a tcg_out_arith_imm, in the similar way we would do 
tcg_out_arith? (tcg_out_arith_reg?)

>  static inline void tcg_out_nop(TCGContext *s)
>  {
>      tcg_out32(s, 0xd503201f);
> @@ -896,7 +937,7 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg 
> addr_reg,
>                   (is_read ? offsetof(CPUTLBEntry, addr_read)
>                    : offsetof(CPUTLBEntry, addr_write)));
>      /* Perform the address comparison. */
> -    tcg_out_cmp(s, (TARGET_LONG_BITS == 64), TCG_REG_X0, TCG_REG_X3);
> +    tcg_out_cmp(s, (TARGET_LONG_BITS == 64), TCG_REG_X0, TCG_REG_X3, 0);
>      *label_ptr = s->code_ptr;
>      /* If not equal, we jump to the slow path. */
>      tcg_out_goto_cond_noaddr(s, TCG_COND_NE);
> @@ -1157,14 +1198,26 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>                       a0, a1, a2);
>          break;
>  
> -    case INDEX_op_add_i64:
>      case INDEX_op_add_i32:
> -        tcg_fmt_Rdnm(s, INSN_ADD, ext, a0, a1, a2);
> +        a2 = (int32_t)a2;
> +        /* FALLTHRU */

/* fall through */ is less ugly.

> +    case INDEX_op_add_i64:
> +        if (c2) {
> +            tcg_out_addsubi(s, ext, a0, a1, a2);
> +        } else {
> +            tcg_fmt_Rdnm(s, INSN_ADD, ext, a0, a1, a2);
> +        }
>          break;
>  
> -    case INDEX_op_sub_i64:
>      case INDEX_op_sub_i32:
> -        tcg_fmt_Rdnm(s, INSN_SUB, ext, a0, a1, a2);
> +        a2 = (int32_t)a2;
> +        /* FALLTHRU */
> +    case INDEX_op_sub_i64:
> +        if (c2) {
> +            tcg_out_addsubi(s, ext, a0, a1, -a2);
> +        } else {
> +            tcg_fmt_Rdnm(s, INSN_SUB, ext, a0, a1, a2);
> +        }
>          break;
>  
>      case INDEX_op_and_i64:
> @@ -1233,15 +1286,19 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          }
>          break;
>  
> -    case INDEX_op_brcond_i64:
>      case INDEX_op_brcond_i32:
> -        tcg_out_cmp(s, ext, a0, a1);
> +        a1 = (int32_t)a1;
> +        /* FALLTHRU */
> +    case INDEX_op_brcond_i64:
> +        tcg_out_cmp(s, ext, a0, a1, const_args[1]);
>          tcg_out_goto_label_cond(s, a2, args[3]);
>          break;
>  
> -    case INDEX_op_setcond_i64:
>      case INDEX_op_setcond_i32:
> -        tcg_out_cmp(s, ext, a1, a2);
> +        a2 = (int32_t)a2;
> +        /* FALLTHRU */
> +    case INDEX_op_setcond_i64:
> +        tcg_out_cmp(s, ext, a1, a2, c2);
>          tcg_out_cset(s, 0, a0, args[3]);
>          break;
>  
> @@ -1362,10 +1419,10 @@ static const TCGTargetOpDef aarch64_op_defs[] = {
>      { INDEX_op_st32_i64, { "r", "r" } },
>      { INDEX_op_st_i64, { "r", "r" } },
>  
> -    { INDEX_op_add_i32, { "r", "r", "r" } },
> -    { INDEX_op_add_i64, { "r", "r", "r" } },
> -    { INDEX_op_sub_i32, { "r", "r", "r" } },
> -    { INDEX_op_sub_i64, { "r", "r", "r" } },
> +    { INDEX_op_add_i32, { "r", "r", "rwA" } },
> +    { INDEX_op_add_i64, { "r", "r", "rA" } },
> +    { INDEX_op_sub_i32, { "r", "r", "rwA" } },
> +    { INDEX_op_sub_i64, { "r", "r", "rA" } },
>      { INDEX_op_mul_i32, { "r", "r", "r" } },
>      { INDEX_op_mul_i64, { "r", "r", "r" } },
>      { INDEX_op_and_i32, { "r", "r", "r" } },
> @@ -1386,10 +1443,10 @@ static const TCGTargetOpDef aarch64_op_defs[] = {
>      { INDEX_op_rotl_i64, { "r", "r", "ri" } },
>      { INDEX_op_rotr_i64, { "r", "r", "ri" } },
>  
> -    { INDEX_op_brcond_i32, { "r", "r" } },
> -    { INDEX_op_setcond_i32, { "r", "r", "r" } },
> -    { INDEX_op_brcond_i64, { "r", "r" } },
> -    { INDEX_op_setcond_i64, { "r", "r", "r" } },
> +    { INDEX_op_brcond_i32, { "r", "rwA" } },
> +    { INDEX_op_brcond_i64, { "r", "rA" } },
> +    { INDEX_op_setcond_i32, { "r", "r", "rwA" } },
> +    { INDEX_op_setcond_i64, { "r", "r", "rA" } },
>  
>      { INDEX_op_qemu_ld8u, { "r", "l" } },
>      { INDEX_op_qemu_ld8s, { "r", "l" } },
>