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

[Richard Henderson <richard.henderson@linaro.org>]

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~