| Message ID | 20221209020530.396391-26-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg/s390x: misc patches | expand |
On Thu, Dec 08, 2022 at 08:05:28PM -0600, Richard Henderson wrote: > Give 64-bit comparison second operand a signed 33-bit immediate. > This is the smallest superset of uint32_t and int32_t, as used > by CLGFI and CGFI respectively. The rest of the 33-bit space > can be loaded into TCG_TMP0. Drop use of the constant pool. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/s390x/tcg-target-con-set.h | 3 +++ > tcg/s390x/tcg-target.c.inc | 27 ++++++++++++++------------- > 2 files changed, 17 insertions(+), 13 deletions(-) <...> > --- a/tcg/s390x/tcg-target.c.inc > +++ b/tcg/s390x/tcg-target.c.inc > @@ -1249,22 +1249,20 @@ static int tgen_cmp2(TCGContext *s, TCGType type, TCGCond c, TCGReg r1, > tcg_out_insn_RIL(s, op, r1, c2); > goto exit; > } > + > + /* > + * Constraints are for a signed 33-bit operand, which is a > + * convenient superset of this signed/unsigned test. > + */ > if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) { > op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); > tcg_out_insn_RIL(s, op, r1, c2); > goto exit; > } > > - /* Use the constant pool, but not for small constants. */ > - if (maybe_out_small_movi(s, type, TCG_TMP0, c2)) { > - c2 = TCG_TMP0; > - /* fall through to reg-reg */ > - } else { > - op = (is_unsigned ? RIL_CLGRL : RIL_CGRL); > - tcg_out_insn_RIL(s, op, r1, 0); > - new_pool_label(s, c2, R_390_PC32DBL, s->code_ptr - 2, 2); > - goto exit; > - } > + /* Load everything else into a register. */ > + tcg_out_movi(s, TCG_TYPE_I64, TCG_TMP0, c2); > + c2 = TCG_TMP0; What does tightening the constraint give us, if we have to handle the "everything else" case anyway, even for values that match TCG_CT_CONST_S33? The example that I have in mind is: - Comparison: r0_64 s<= -0xffffffffL; - tcg_target_const_match(-0xffffffffL, TCG_TYPE_I64, TCG_CT_CONST_S33) == true; - (long)(int)-0xffffffffL != -0xffffffff; - So we should end up in the "everything else" branch. <...> Best regards, Ilya
On 12/13/22 10:25, Ilya Leoshkevich wrote: > On Thu, Dec 08, 2022 at 08:05:28PM -0600, Richard Henderson wrote: >> Give 64-bit comparison second operand a signed 33-bit immediate. >> This is the smallest superset of uint32_t and int32_t, as used >> by CLGFI and CGFI respectively. The rest of the 33-bit space >> can be loaded into TCG_TMP0. Drop use of the constant pool. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> tcg/s390x/tcg-target-con-set.h | 3 +++ >> tcg/s390x/tcg-target.c.inc | 27 ++++++++++++++------------- >> 2 files changed, 17 insertions(+), 13 deletions(-) > > <...> > >> --- a/tcg/s390x/tcg-target.c.inc >> +++ b/tcg/s390x/tcg-target.c.inc >> @@ -1249,22 +1249,20 @@ static int tgen_cmp2(TCGContext *s, TCGType type, TCGCond c, TCGReg r1, >> tcg_out_insn_RIL(s, op, r1, c2); >> goto exit; >> } >> + >> + /* >> + * Constraints are for a signed 33-bit operand, which is a >> + * convenient superset of this signed/unsigned test. >> + */ >> if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) { >> op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); >> tcg_out_insn_RIL(s, op, r1, c2); >> goto exit; >> } >> >> - /* Use the constant pool, but not for small constants. */ >> - if (maybe_out_small_movi(s, type, TCG_TMP0, c2)) { >> - c2 = TCG_TMP0; >> - /* fall through to reg-reg */ >> - } else { >> - op = (is_unsigned ? RIL_CLGRL : RIL_CGRL); >> - tcg_out_insn_RIL(s, op, r1, 0); >> - new_pool_label(s, c2, R_390_PC32DBL, s->code_ptr - 2, 2); >> - goto exit; >> - } >> + /* Load everything else into a register. */ >> + tcg_out_movi(s, TCG_TYPE_I64, TCG_TMP0, c2); >> + c2 = TCG_TMP0; > > What does tightening the constraint give us, if we have to handle the > "everything else" case anyway, even for values that match > TCG_CT_CONST_S33? Values outside const_s33 get loaded by the register allocator, which means the value in the register might get re-used. > The example that I have in mind is: > > - Comparison: r0_64 s<= -0xffffffffL; > - tcg_target_const_match(-0xffffffffL, TCG_TYPE_I64, > TCG_CT_CONST_S33) == true; > - (long)(int)-0xffffffffL != -0xffffffff; > - So we should end up in the "everything else" branch. I suppose I could invent a new constraint that matches INT_MIN32 .. UINT32_MAX, which would exclude this particular case. But it would still leave us loading INT32MIN .. -1 for unsigned and INT32MAX+1 .. UINT32_MAX for signed. Since S33 existed already, I thought I would just re-use it. r~
On Tue, Dec 13, 2022 at 10:43:07AM -0600, Richard Henderson wrote: > On 12/13/22 10:25, Ilya Leoshkevich wrote: > > On Thu, Dec 08, 2022 at 08:05:28PM -0600, Richard Henderson wrote: > > > Give 64-bit comparison second operand a signed 33-bit immediate. > > > This is the smallest superset of uint32_t and int32_t, as used > > > by CLGFI and CGFI respectively. The rest of the 33-bit space > > > can be loaded into TCG_TMP0. Drop use of the constant pool. > > > > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > > --- > > > tcg/s390x/tcg-target-con-set.h | 3 +++ > > > tcg/s390x/tcg-target.c.inc | 27 ++++++++++++++------------- > > > 2 files changed, 17 insertions(+), 13 deletions(-) > > > > <...> > > > --- a/tcg/s390x/tcg-target.c.inc > > > +++ b/tcg/s390x/tcg-target.c.inc > > > @@ -1249,22 +1249,20 @@ static int tgen_cmp2(TCGContext *s, TCGType type, TCGCond c, TCGReg r1, > > > tcg_out_insn_RIL(s, op, r1, c2); > > > goto exit; > > > } > > > + > > > + /* > > > + * Constraints are for a signed 33-bit operand, which is a > > > + * convenient superset of this signed/unsigned test. > > > + */ > > > if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) { > > > op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); > > > tcg_out_insn_RIL(s, op, r1, c2); > > > goto exit; > > > } > > > - /* Use the constant pool, but not for small constants. */ > > > - if (maybe_out_small_movi(s, type, TCG_TMP0, c2)) { > > > - c2 = TCG_TMP0; > > > - /* fall through to reg-reg */ > > > - } else { > > > - op = (is_unsigned ? RIL_CLGRL : RIL_CGRL); > > > - tcg_out_insn_RIL(s, op, r1, 0); > > > - new_pool_label(s, c2, R_390_PC32DBL, s->code_ptr - 2, 2); > > > - goto exit; > > > - } > > > + /* Load everything else into a register. */ > > > + tcg_out_movi(s, TCG_TYPE_I64, TCG_TMP0, c2); > > > + c2 = TCG_TMP0; > > > > What does tightening the constraint give us, if we have to handle the > > "everything else" case anyway, even for values that match > > TCG_CT_CONST_S33? > > Values outside const_s33 get loaded by the register allocator, which means > the value in the register might get re-used. Thanks for the explanation! I did not consider the reuse of already loaded large 64-bit values. Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
diff --git a/tcg/s390x/tcg-target-con-set.h b/tcg/s390x/tcg-target-con-set.h index baf3bc9037..15f1c55103 100644 --- a/tcg/s390x/tcg-target-con-set.h +++ b/tcg/s390x/tcg-target-con-set.h @@ -13,6 +13,7 @@ C_O0_I1(r) C_O0_I2(L, L) C_O0_I2(r, r) C_O0_I2(r, ri) +C_O0_I2(r, rA) C_O0_I2(v, r) C_O1_I1(r, L) C_O1_I1(r, r) @@ -24,6 +25,7 @@ C_O1_I2(r, 0, rI) C_O1_I2(r, 0, rJ) C_O1_I2(r, r, r) C_O1_I2(r, r, ri) +C_O1_I2(r, r, rA) C_O1_I2(r, r, rI) C_O1_I2(r, r, rJ) C_O1_I2(r, r, rK) @@ -35,6 +37,7 @@ C_O1_I2(v, v, r) C_O1_I2(v, v, v) C_O1_I3(v, v, v, v) C_O1_I4(r, r, ri, rI, r) +C_O1_I4(r, r, rA, rI, r) C_O2_I2(o, m, 0, r) C_O2_I2(o, m, r, r) C_O2_I3(o, m, 0, 1, r) diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc index c0434fa2f8..4d113139e5 100644 --- a/tcg/s390x/tcg-target.c.inc +++ b/tcg/s390x/tcg-target.c.inc @@ -1249,22 +1249,20 @@ static int tgen_cmp2(TCGContext *s, TCGType type, TCGCond c, TCGReg r1, tcg_out_insn_RIL(s, op, r1, c2); goto exit; } + + /* + * Constraints are for a signed 33-bit operand, which is a + * convenient superset of this signed/unsigned test. + */ if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) { op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); tcg_out_insn_RIL(s, op, r1, c2); goto exit; } - /* Use the constant pool, but not for small constants. */ - if (maybe_out_small_movi(s, type, TCG_TMP0, c2)) { - c2 = TCG_TMP0; - /* fall through to reg-reg */ - } else { - op = (is_unsigned ? RIL_CLGRL : RIL_CGRL); - tcg_out_insn_RIL(s, op, r1, 0); - new_pool_label(s, c2, R_390_PC32DBL, s->code_ptr - 2, 2); - goto exit; - } + /* Load everything else into a register. */ + tcg_out_movi(s, TCG_TYPE_I64, TCG_TMP0, c2); + c2 = TCG_TMP0; } if (type == TCG_TYPE_I32) { @@ -3105,8 +3103,9 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op) case INDEX_op_rotr_i32: case INDEX_op_rotr_i64: case INDEX_op_setcond_i32: - case INDEX_op_setcond_i64: return C_O1_I2(r, r, ri); + case INDEX_op_setcond_i64: + return C_O1_I2(r, r, rA); case INDEX_op_clz_i64: return C_O1_I2(r, r, rI); @@ -3154,8 +3153,9 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op) return C_O1_I2(r, r, ri); case INDEX_op_brcond_i32: - case INDEX_op_brcond_i64: return C_O0_I2(r, ri); + case INDEX_op_brcond_i64: + return C_O0_I2(r, rA); case INDEX_op_bswap16_i32: case INDEX_op_bswap16_i64: @@ -3196,8 +3196,9 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op) return C_O1_I2(r, rZ, r); case INDEX_op_movcond_i32: - case INDEX_op_movcond_i64: return C_O1_I4(r, r, ri, rI, r); + case INDEX_op_movcond_i64: + return C_O1_I4(r, r, rA, rI, r); case INDEX_op_div2_i32: case INDEX_op_div2_i64:
Give 64-bit comparison second operand a signed 33-bit immediate. This is the smallest superset of uint32_t and int32_t, as used by CLGFI and CGFI respectively. The rest of the 33-bit space can be loaded into TCG_TMP0. Drop use of the constant pool. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/s390x/tcg-target-con-set.h | 3 +++ tcg/s390x/tcg-target.c.inc | 27 ++++++++++++++------------- 2 files changed, 17 insertions(+), 13 deletions(-)