1. Patchew
  2. QEMU
  3. View series

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

Richard Henderson posted 1 patch 3 months, 2 weeks ago 
tcg/ppc/tcg-target.c.inc | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
[PATCH for-9.1] tcg/ppc: Sync tcg_out_test and constraints
Posted by Richard Henderson 3 months, 2 weeks ago 
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);
-- 
2.43.0
Re: [PATCH for-9.1] tcg/ppc: Sync tcg_out_test and constraints
Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago 
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>
Re: [PATCH for-9.1] tcg/ppc: Sync tcg_out_test and constraints
Posted by Nicholas Piggin 3 months, 2 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.