Re: [PATCH v4 25/27] tcg/s390x: Tighten constraints for 64-bit compare

[Ilya Leoshkevich]

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>