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.hender...@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 <i...@linux.ibm.com>
