| Message ID | 1348685335-16770-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 26 September 2012 20:01, Richard Henderson <rth@twiddle.net> wrote: > On 09/26/2012 11:48 AM, Peter Maydell wrote: >> case INDEX_op_setcond_i32: >> - if (const_args[2]) { >> - int rot; >> - rot = encode_imm(args[2]); >> - tcg_out_dat_imm(s, COND_AL, ARITH_CMP, 0, >> - args[1], rotl(args[2], rot) | (rot << 7)); >> - } else { >> - tcg_out_dat_reg(s, COND_AL, ARITH_CMP, 0, >> - args[1], args[2], SHIFT_IMM_LSL(0)); >> - } >> + tcg_out_dat_rI(s, COND_AL, ARITH_CMP, 0, >> + args[1], args[2], const_args[2]); >> tcg_out_dat_imm(s, tcg_cond_to_arm_cond[args[3]], >> ARITH_MOV, args[0], 0, 1); >> tcg_out_dat_imm(s, tcg_cond_to_arm_cond[tcg_invert_cond(args[3])], > > The patch itself is fine. But as a followup, if movcc is no longer "free", > then perhaps the setcond sequence is better as > > cmp > mov > movcc > > i.e. the second move is unconditional? A register renaming OOO core could > then schedule the mov before the cmp. Well, maybe. But my bar for changing existing code requires more proof that it's worth making the change (basically in both cases I'm optimising for convenience of development). My suspicion (entirely unbenchmarked) is that at the moment both these sequences will pan out about the same cost, so we might as well pick the one that's easiest to code. -- PMM
On 26 September 2012 20:46, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 September 2012 20:01, Richard Henderson <rth@twiddle.net> wrote: >> The patch itself is fine. But as a followup, if movcc is no longer "free", >> then perhaps the setcond sequence is better as >> >> cmp >> mov >> movcc >> >> i.e. the second move is unconditional? A register renaming OOO core could >> then schedule the mov before the cmp. > > Well, maybe. But my bar for changing existing code requires more > proof that it's worth making the change (basically in both cases > I'm optimising for convenience of development). My suspicion > (entirely unbenchmarked) is that at the moment both these sequences > will pan out about the same cost, so we might as well pick the > one that's easiest to code. Apparently there is really very little in it but if we did change then the recommendation would be to try something like: MOV dest, #0 CMP c1, c2 ADDcc dest, dest, #1 I could tell you why, but then I'd have to kill you :-) -- PMM
On Wed, Sep 26, 2012 at 07:48:54PM +0100, Peter Maydell wrote: > The code to emit either an immediate cmp or a register cmp insn is > duplicated in several places; factor it out into its own function. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > tcg/arm/tcg-target.c | 46 ++++++++++++++++++++-------------------------- > 1 file changed, 20 insertions(+), 26 deletions(-) > > diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c > index 2bad0a2..a83b295 100644 > --- a/tcg/arm/tcg-target.c > +++ b/tcg/arm/tcg-target.c > @@ -467,6 +467,21 @@ static inline void tcg_out_movi32(TCGContext *s, > } > } > > +static inline void tcg_out_dat_rI(TCGContext *s, int cond, int opc, TCGArg dst, > + TCGArg lhs, TCGArg rhs, int rhs_is_const) > +{ > + /* Emit either the reg,imm or reg,reg form of a data-processing insn. > + * rhs must satisfy the "rI" constraint. > + */ > + if (rhs_is_const) { > + int rot = encode_imm(rhs); > + assert(rot >= 0); > + tcg_out_dat_imm(s, cond, opc, dst, lhs, rotl(rhs, rot) | (rot << 7)); > + } else { > + tcg_out_dat_reg(s, cond, opc, dst, lhs, rhs, SHIFT_IMM_LSL(0)); > + } > +} > + > static inline void tcg_out_mul32(TCGContext *s, > int cond, int rd, int rs, int rm) > { > @@ -1591,14 +1606,7 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > c = ARITH_EOR; > /* Fall through. */ > gen_arith: > - if (const_args[2]) { > - int rot; > - rot = encode_imm(args[2]); > - tcg_out_dat_imm(s, COND_AL, c, > - args[0], args[1], rotl(args[2], rot) | (rot << 7)); > - } else > - tcg_out_dat_reg(s, COND_AL, c, > - args[0], args[1], args[2], SHIFT_IMM_LSL(0)); > + tcg_out_dat_rI(s, COND_AL, c, args[0], args[1], args[2], const_args[2]); > break; > case INDEX_op_add2_i32: > tcg_out_dat_reg2(s, COND_AL, ARITH_ADD, ARITH_ADC, > @@ -1658,15 +1666,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > break; > > case INDEX_op_brcond_i32: > - if (const_args[1]) { > - int rot; > - rot = encode_imm(args[1]); > - tcg_out_dat_imm(s, COND_AL, ARITH_CMP, 0, > - args[0], rotl(args[1], rot) | (rot << 7)); > - } else { > - tcg_out_dat_reg(s, COND_AL, ARITH_CMP, 0, > - args[0], args[1], SHIFT_IMM_LSL(0)); > - } > + tcg_out_dat_rI(s, COND_AL, ARITH_CMP, 0, > + args[0], args[1], const_args[1]); > tcg_out_goto_label(s, tcg_cond_to_arm_cond[args[2]], args[3]); > break; > case INDEX_op_brcond2_i32: > @@ -1685,15 +1686,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > tcg_out_goto_label(s, tcg_cond_to_arm_cond[args[4]], args[5]); > break; > case INDEX_op_setcond_i32: > - if (const_args[2]) { > - int rot; > - rot = encode_imm(args[2]); > - tcg_out_dat_imm(s, COND_AL, ARITH_CMP, 0, > - args[1], rotl(args[2], rot) | (rot << 7)); > - } else { > - tcg_out_dat_reg(s, COND_AL, ARITH_CMP, 0, > - args[1], args[2], SHIFT_IMM_LSL(0)); > - } > + tcg_out_dat_rI(s, COND_AL, ARITH_CMP, 0, > + args[1], args[2], const_args[2]); > tcg_out_dat_imm(s, tcg_cond_to_arm_cond[args[3]], > ARITH_MOV, args[0], 0, 1); > tcg_out_dat_imm(s, tcg_cond_to_arm_cond[tcg_invert_cond(args[3])], Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c index 2bad0a2..a83b295 100644 --- a/tcg/arm/tcg-target.c +++ b/tcg/arm/tcg-target.c @@ -467,6 +467,21 @@ static inline void tcg_out_movi32(TCGContext *s, } } +static inline void tcg_out_dat_rI(TCGContext *s, int cond, int opc, TCGArg dst, + TCGArg lhs, TCGArg rhs, int rhs_is_const) +{ + /* Emit either the reg,imm or reg,reg form of a data-processing insn. + * rhs must satisfy the "rI" constraint. + */ + if (rhs_is_const) { + int rot = encode_imm(rhs); + assert(rot >= 0); + tcg_out_dat_imm(s, cond, opc, dst, lhs, rotl(rhs, rot) | (rot << 7)); + } else { + tcg_out_dat_reg(s, cond, opc, dst, lhs, rhs, SHIFT_IMM_LSL(0)); + } +} + static inline void tcg_out_mul32(TCGContext *s, int cond, int rd, int rs, int rm) { @@ -1591,14 +1606,7 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, c = ARITH_EOR; /* Fall through. */ gen_arith: - if (const_args[2]) { - int rot; - rot = encode_imm(args[2]); - tcg_out_dat_imm(s, COND_AL, c, - args[0], args[1], rotl(args[2], rot) | (rot << 7)); - } else - tcg_out_dat_reg(s, COND_AL, c, - args[0], args[1], args[2], SHIFT_IMM_LSL(0)); + tcg_out_dat_rI(s, COND_AL, c, args[0], args[1], args[2], const_args[2]); break; case INDEX_op_add2_i32: tcg_out_dat_reg2(s, COND_AL, ARITH_ADD, ARITH_ADC, @@ -1658,15 +1666,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, break; case INDEX_op_brcond_i32: - if (const_args[1]) { - int rot; - rot = encode_imm(args[1]); - tcg_out_dat_imm(s, COND_AL, ARITH_CMP, 0, - args[0], rotl(args[1], rot) | (rot << 7)); - } else { - tcg_out_dat_reg(s, COND_AL, ARITH_CMP, 0, - args[0], args[1], SHIFT_IMM_LSL(0)); - } + tcg_out_dat_rI(s, COND_AL, ARITH_CMP, 0, + args[0], args[1], const_args[1]); tcg_out_goto_label(s, tcg_cond_to_arm_cond[args[2]], args[3]); break; case INDEX_op_brcond2_i32: @@ -1685,15 +1686,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, tcg_out_goto_label(s, tcg_cond_to_arm_cond[args[4]], args[5]); break; case INDEX_op_setcond_i32: - if (const_args[2]) { - int rot; - rot = encode_imm(args[2]); - tcg_out_dat_imm(s, COND_AL, ARITH_CMP, 0, - args[1], rotl(args[2], rot) | (rot << 7)); - } else { - tcg_out_dat_reg(s, COND_AL, ARITH_CMP, 0, - args[1], args[2], SHIFT_IMM_LSL(0)); - } + tcg_out_dat_rI(s, COND_AL, ARITH_CMP, 0, + args[1], args[2], const_args[2]); tcg_out_dat_imm(s, tcg_cond_to_arm_cond[args[3]], ARITH_MOV, args[0], 0, 1); tcg_out_dat_imm(s, tcg_cond_to_arm_cond[tcg_invert_cond(args[3])],
The code to emit either an immediate cmp or a register cmp insn is duplicated in several places; factor it out into its own function. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- tcg/arm/tcg-target.c | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-)