Re: [PATCH 4/4] tcg/optimize: optimize TSTNE using smask and zmask

[Richard Henderson]

On 2/28/24 23:35, Paolo Bonzini wrote:
> On 2/29/24 00:10, Richard Henderson wrote:
> > On 2/28/24 01:11, Paolo Bonzini wrote:
> > > -    /* TSTNE x,sign -> LT x,0 */
> > > -    if (arg_is_const_val(*p2, (ctx->type == TCG_TYPE_I32
> > > -                               ? INT32_MIN : INT64_MIN))) {
> > > +    /* TSTNE x,i -> LT x,0 if i only includes sign bit copies */
> > > +    if (arg_is_const(*p2) && (arg_info(*p2)->val & ~i1->s_mask) == 0) {
> >
> > This is a good idea, but s_mask isn't defined like you think -- it is *repetitions* of 
> > the sign bit, but not including the sign bit itself. For INT64_MIN, s_mask == 0.
> >
> > So for TSTNE min,min, (min & ~0) != 0, so the test won't pass.
>
> Oh! So I have to squash:
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index ab976a5bbe7..44d1b1a6d8a 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -140,6 +140,12 @@ static inline bool arg_is_const_val(TCGArg arg, uint64_t 
> val)
>       return ts_is_const_val(arg_temp(arg), val);
>   }
>
> +/* Calculate all the copies of the sign bit, both redundant and not. */
> +static inline uint64_t all_sign_bit_copies(TempOptInfo *info)
> +{
> +    return (info->s_mask >> 1) | INT64_MIN;
> +}

You need to care about type too -- for TCG_TYPE_I32, you'll want to OR in INT32_MIN.  The 
high bits of s_mask will be unknown (might be 1's from fold_masks, might be 0 from reset_ts).

But otherwise that's a good solution.


r~