[PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[Paolo Bonzini]

32-bit TSTEQ and TSTNE is subject to the same constraints as
for 64-bit, but setcond_i32 and negsetcond_i32 were incorrectly
using TCG_CT_CONST ("i") instead of TCG_CT_CONST_CMP ("C").

Adjust the constraint and make tcg_target_const_match use the
same sequence as tgen_cmp2: first check if the constant is a
valid operand for TSTEQ/TSTNE, then accept everything for 32-bit
non-test comparisons, finally check if the constant is a valid
operand for 64-bit non-test comparisons.

Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tcg/s390x/tcg-target.c.inc | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
index a5d57197a4b..27bccc14e50 100644
--- a/tcg/s390x/tcg-target.c.inc
+++ b/tcg/s390x/tcg-target.c.inc
@@ -565,6 +565,20 @@ static bool tcg_target_const_match(int64_t val, int ct,
     }
 
     if (ct & TCG_CT_CONST_CMP) {
+        if (is_tst_cond(cond)) {
+            if (is_const_p16(uval) >= 0) {
+                return true;  /* TMxx */
+            }
+            if (risbg_mask(uval)) {
+                return true;  /* RISBG */
+            }
+            return false;
+        }
+
+        if (type == TCG_TYPE_I32) {
+            return true;
+        }
+
         switch (cond) {
         case TCG_COND_EQ:
         case TCG_COND_NE:
@@ -584,13 +598,7 @@ static bool tcg_target_const_match(int64_t val, int ct,
             break;
         case TCG_COND_TSTNE:
         case TCG_COND_TSTEQ:
-            if (is_const_p16(uval) >= 0) {
-                return true;  /* TMxx */
-            }
-            if (risbg_mask(uval)) {
-                return true;  /* RISBG */
-            }
-            break;
+            /* checked above, fallthru */
         default:
             g_assert_not_reached();
         }
@@ -3231,9 +3239,9 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op)
     case INDEX_op_rotl_i64:
     case INDEX_op_rotr_i32:
     case INDEX_op_rotr_i64:
+        return C_O1_I2(r, r, ri);
     case INDEX_op_setcond_i32:
     case INDEX_op_negsetcond_i32:
-        return C_O1_I2(r, r, ri);
     case INDEX_op_setcond_i64:
     case INDEX_op_negsetcond_i64:
         return C_O1_I2(r, r, rC);
-- 
2.46.2

Re: [PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[Peter Maydell]

On Thu, 17 Oct 2024 at 10:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> 32-bit TSTEQ and TSTNE is subject to the same constraints as
> for 64-bit, but setcond_i32 and negsetcond_i32 were incorrectly
> using TCG_CT_CONST ("i") instead of TCG_CT_CONST_CMP ("C").
>
> Adjust the constraint and make tcg_target_const_match use the
> same sequence as tgen_cmp2: first check if the constant is a
> valid operand for TSTEQ/TSTNE, then accept everything for 32-bit
> non-test comparisons, finally check if the constant is a valid
> operand for 64-bit non-test comparisons.
>
> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Should this cc stable? Does it cause any current problems?
(AIUI the x86 target changes in your pending pullreq do
trigger this.)

thanks
-- PMM

Re: [PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[Paolo Bonzini]

On 10/17/24 12:00, Peter Maydell wrote:
> On Thu, 17 Oct 2024 at 10:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> 32-bit TSTEQ and TSTNE is subject to the same constraints as
>> for 64-bit, but setcond_i32 and negsetcond_i32 were incorrectly
>> using TCG_CT_CONST ("i") instead of TCG_CT_CONST_CMP ("C").
>>
>> Adjust the constraint and make tcg_target_const_match use the
>> same sequence as tgen_cmp2: first check if the constant is a
>> valid operand for TSTEQ/TSTNE, then accept everything for 32-bit
>> non-test comparisons, finally check if the constant is a valid
>> operand for 64-bit non-test comparisons.
>>
>> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Should this cc stable? Does it cause any current problems?
> (AIUI the x86 target changes in your pending pullreq do
> trigger this.)

Yeah, that's a good idea.  It's probably possible to construct x86 code 
that triggers it (I'm surprised it wasn't found until now).

Paolo

Re: [PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[Richard Henderson]

On 10/17/24 02:14, Paolo Bonzini wrote:
> 32-bit TSTEQ and TSTNE is subject to the same constraints as
> for 64-bit, but setcond_i32 and negsetcond_i32 were incorrectly
> using TCG_CT_CONST ("i") instead of TCG_CT_CONST_CMP ("C").
> 
> Adjust the constraint and make tcg_target_const_match use the
> same sequence as tgen_cmp2: first check if the constant is a
> valid operand for TSTEQ/TSTNE, then accept everything for 32-bit
> non-test comparisons, finally check if the constant is a valid
> operand for 64-bit non-test comparisons.
> 
> Reported-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   tcg/s390x/tcg-target.c.inc | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] tcg/s390x: fix constraint for 32-bit TSTEQ/TSTNE

[Philippe Mathieu-Daudé]

On 17/10/24 06:14, Paolo Bonzini wrote:
> 32-bit TSTEQ and TSTNE is subject to the same constraints as
> for 64-bit, but setcond_i32 and negsetcond_i32 were incorrectly
> using TCG_CT_CONST ("i") instead of TCG_CT_CONST_CMP ("C").
> 
> Adjust the constraint and make tcg_target_const_match use the
> same sequence as tgen_cmp2: first check if the constant is a
> valid operand for TSTEQ/TSTNE, then accept everything for 32-bit
> non-test comparisons, finally check if the constant is a valid
> operand for 64-bit non-test comparisons.
> 
> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tcg/s390x/tcg-target.c.inc | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>