| Message ID | 1443627027-2193-7-git-send-email-james.hogan@imgtec.com |
|---|---|
| State | New |
| Headers | show |
On 10/01/2015 01:30 AM, James Hogan wrote: > Adapt the MIPS movcond implementation to handle the full select > operation using a pair of MOVN/MOVZ instructions. > > This allows the register alias constraint to be removed (which is what > ensured v2 == dest), and allows it to be more easily extended to support > the MIPS r6 instructions SELNEZ/SELEQZ which replace MOVN/MOVZ and > require similar logic. > > For example, previously we only supported: > movcond_i32 dest, c1, c2, v1, v2=dest, cond > > With the host code: > MOV[ZN] dest, v1, [!](c1 cond c2) > > Meaning: > if (c1 cond c2) > dest = v1; > > But now v2 doesn't have to equal dest, so we can support: > movcond_i32 dest, c1, c2, v1, v2, cond > > With the host code: > #if dest != v1 > MOV[ZN] dest, v1, [!](c1 cond c2) > #endif > #if dest != v2 > MOV[NZ] dest, v1,  > #endif > > Meaning: > #if dest != v1 > if ([!](c1 cond c2)) > dest = v1; > #endif > #if dest != v2 > if () > dest = v2; > #endif I don't think this is a good change. In the case of dest != v1 && dest != v2, we wind up with two conditional instructions. On most targets that I'm familiar with, this is more expensive than a plain move. If r6 was conditional, as opposed to something that we must perforce know about at compilation time, then I'd say go ahead but split this into a normal move followed by a conditional move. But since that's not the case, I don't see the point in changing anything at all. r~
Hi Richard, On Thu, Oct 01, 2015 at 06:56:39AM +1000, Richard Henderson wrote: > On 10/01/2015 01:30 AM, James Hogan wrote: > > Adapt the MIPS movcond implementation to handle the full select > > operation using a pair of MOVN/MOVZ instructions. > > > > This allows the register alias constraint to be removed (which is what > > ensured v2 == dest), and allows it to be more easily extended to support > > the MIPS r6 instructions SELNEZ/SELEQZ which replace MOVN/MOVZ and > > require similar logic. > > > > For example, previously we only supported: > > movcond_i32 dest, c1, c2, v1, v2=dest, cond > > > > With the host code: > > MOV[ZN] dest, v1, [!](c1 cond c2) > > > > Meaning: > > if (c1 cond c2) > > dest = v1; > > > > But now v2 doesn't have to equal dest, so we can support: > > movcond_i32 dest, c1, c2, v1, v2, cond > > > > With the host code: > > #if dest != v1 > > MOV[ZN] dest, v1, [!](c1 cond c2) > > #endif > > #if dest != v2 > > MOV[NZ] dest, v1,  > > #endif > > > > Meaning: > > #if dest != v1 > > if ([!](c1 cond c2)) > > dest = v1; > > #endif > > #if dest != v2 > > if () > > dest = v2; > > #endif > > I don't think this is a good change. In the case of dest != v1 && dest != v2, > we wind up with two conditional instructions. On most targets that I'm > familiar with, this is more expensive than a plain move. > > If r6 was conditional, as opposed to something that we must perforce know about > at compilation time, then I'd say go ahead but split this into a normal move > followed by a conditional move. But since that's not the case, I don't see the > point in changing anything at all. Yeh, I think you're right. This is certainly the change I was struggling to convince myself about it being worthwhile, since it was unclear to me just what net effect the operand aliasing constraint would have on register allocation and the generated code. I'll resubmit my original version without this patch. R2/R6 will just have different constraints via an ifdef so that the R2 code doesn't have to deal with that case (since dest will always be v2). Thanks for the review! Cheers James
diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c index c447db6011ea..9849896bd75b 100644 --- a/tcg/mips/tcg-target.c +++ b/tcg/mips/tcg-target.c @@ -852,13 +852,17 @@ static void tcg_out_brcond2(TCGContext *s, TCGCond cond, TCGReg al, TCGReg ah, } static void tcg_out_movcond(TCGContext *s, TCGCond cond, TCGReg ret, - TCGReg c1, TCGReg c2, TCGReg v) + TCGReg c1, TCGReg c2, TCGReg v1, TCGReg v2) { - MIPSInsn m_opc = OPC_MOVN; + MIPSInsn m_opc_t = OPC_MOVN; + MIPSInsn m_opc_f = OPC_MOVZ; + const MIPSInsn m_opc_t_inv = m_opc_f; + const MIPSInsn m_opc_f_inv = m_opc_t; switch (cond) { case TCG_COND_EQ: - m_opc = OPC_MOVZ; + m_opc_t = m_opc_t_inv; + m_opc_f = m_opc_f_inv; /* FALLTHRU */ case TCG_COND_NE: if (c2 != 0) { @@ -871,14 +875,20 @@ static void tcg_out_movcond(TCGContext *s, TCGCond cond, TCGReg ret, /* Minimize code size by preferring a compare not requiring INV. */ if (mips_cmp_map[cond] & MIPS_CMP_INV) { cond = tcg_invert_cond(cond); - m_opc = OPC_MOVZ; + m_opc_t = m_opc_t_inv; + m_opc_f = m_opc_f_inv; } tcg_out_setcond(s, cond, TCG_TMP0, c1, c2); c1 = TCG_TMP0; break; } - tcg_out_opc_reg(s, m_opc, ret, v, c1); + if (v1 != ret) { + tcg_out_opc_reg(s, m_opc_t, ret, v1, c1); + } + if (v2 != ret) { + tcg_out_opc_reg(s, m_opc_f, ret, v2, c1); + } } static void tcg_out_call_int(TCGContext *s, tcg_insn_unit *arg, bool tail) @@ -1575,7 +1585,7 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, break; case INDEX_op_movcond_i32: - tcg_out_movcond(s, args[5], a0, a1, a2, args[3]); + tcg_out_movcond(s, args[5], a0, a1, a2, args[3], args[4]); break; case INDEX_op_setcond_i32: @@ -1664,7 +1674,7 @@ static const TCGTargetOpDef mips_op_defs[] = { { INDEX_op_deposit_i32, { "r", "0", "rZ" } }, { INDEX_op_brcond_i32, { "rZ", "rZ" } }, - { INDEX_op_movcond_i32, { "r", "rZ", "rZ", "rZ", "0" } }, + { INDEX_op_movcond_i32, { "r", "rZ", "rZ", "rZ", "rZ" } }, { INDEX_op_setcond_i32, { "r", "rZ", "rZ" } }, { INDEX_op_setcond2_i32, { "r", "rZ", "rZ", "rZ", "rZ" } },
Adapt the MIPS movcond implementation to handle the full select operation using a pair of MOVN/MOVZ instructions. This allows the register alias constraint to be removed (which is what ensured v2 == dest), and allows it to be more easily extended to support the MIPS r6 instructions SELNEZ/SELEQZ which replace MOVN/MOVZ and require similar logic. For example, previously we only supported: movcond_i32 dest, c1, c2, v1, v2=dest, cond With the host code: MOV[ZN] dest, v1, [!](c1 cond c2) Meaning: if (c1 cond c2) dest = v1; But now v2 doesn't have to equal dest, so we can support: movcond_i32 dest, c1, c2, v1, v2, cond With the host code: #if dest != v1 MOV[ZN] dest, v1, [!](c1 cond c2) #endif #if dest != v2 MOV[NZ] dest, v1,  #endif Meaning: #if dest != v1 if ([!](c1 cond c2)) dest = v1; #endif #if dest != v2 if () dest = v2; #endif Impact/benefit of this patch on TB average host size for a MIPr6 guest kernel boot was negligible, so it was considered preferable to ifdef'ing the constraint based on the presence of MIPS r6. Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Aurelien Jarno <aurelien@aurel32.net> --- tcg/mips/tcg-target.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)