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

[Richard Henderson]

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~