Re: [PATCH for-9.1] tcg/ppc: Sync tcg_out_test and constraints

["Nicholas Piggin" <npiggin@gmail.com>]

On Thu Aug 8, 2024 at 2:46 AM AEST, Philippe Mathieu-Daudé wrote:
> On 7/8/24 06:08, Richard Henderson wrote:
> > Ensure the code structure is the same for matching constraints
> > and emitting code, lest we allow constants that cannot be
> > trivially tested.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: ad788aebbab ("tcg/ppc: Support TCG_COND_TST{EQ,NE}")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2487
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   tcg/ppc/tcg-target.c.inc | 21 ++++++++++-----------
> >   1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
> > index 7f3829beeb..3553a47ba9 100644
> > --- a/tcg/ppc/tcg-target.c.inc
> > +++ b/tcg/ppc/tcg-target.c.inc
> > @@ -325,9 +325,11 @@ static bool tcg_target_const_match(int64_t sval, int ct,
> >               if ((uval & ~0xffff) == 0 || (uval & ~0xffff0000ull) == 0) {
> >                   return 1;
> >               }
> > -            if (TCG_TARGET_REG_BITS == 32 || type == TCG_TYPE_I32
> > -                ? mask_operand(uval, &mb, &me)
> > -                : mask64_operand(uval << clz64(uval), &mb, &me)) {
> > +            if (uval == (uint32_t)uval && mask_operand(uval, &mb, &me)) {
> > +                return 1;
> > +            }
> > +            if (TCG_TARGET_REG_BITS == 64 &&
> > +                mask64_operand(uval << clz64(uval), &mb, &me)) {
> >                   return 1;
> >               }
> >               return 0;
> > @@ -1749,8 +1751,6 @@ static void tcg_out_test(TCGContext *s, TCGReg dest, TCGReg arg1, TCGArg arg2,
> >   
> >       if (type == TCG_TYPE_I32) {
> >           arg2 = (uint32_t)arg2;
> > -    } else if (arg2 == (uint32_t)arg2) {
> > -        type = TCG_TYPE_I32;
> >       }
> >   
> >       if ((arg2 & ~0xffff) == 0) {
> > @@ -1761,12 +1761,11 @@ static void tcg_out_test(TCGContext *s, TCGReg dest, TCGReg arg1, TCGArg arg2,
> >           tcg_out32(s, ANDIS | SAI(arg1, dest, arg2 >> 16));
> >           return;
> >       }
> > -    if (TCG_TARGET_REG_BITS == 32 || type == TCG_TYPE_I32) {
> > -        if (mask_operand(arg2, &mb, &me)) {
> > -            tcg_out_rlw_rc(s, RLWINM, dest, arg1, 0, mb, me, rc);
> > -            return;
> > -        }
> > -    } else {
> > +    if (arg2 == (uint32_t)arg2 && mask_operand(arg2, &mb, &me)) {
> > +        tcg_out_rlw_rc(s, RLWINM, dest, arg1, 0, mb, me, rc);
> > +        return;
> > +    }
> > +    if (TCG_TARGET_REG_BITS == 64) {
> >           int sh = clz64(arg2);
> >           if (mask64_operand(arg2 << sh, &mb, &me)) {
> >               tcg_out_rld_rc(s, RLDICR, dest, arg1, sh, me, rc);
>
> Preferably having someone from PPC also reviewing this,
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I don't know much of the back end, but I think I follow where the
match was letting through a constant that could not be emitted.
Thanks for the fix and review.

Bigger problem seems to be I'm not testing x86 on ppc so I'll have
to rectify that. Also IIRC we were supposed to be adding a ppc
runner to gitlab CI, I'll have to check what's happening with that,
I think it got stalled on IT security rigmarole :(

Thanks,
Nick