Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x

[David Hildenbrand]

> +static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
> +{
> +    DisasCompare c;
> +    disas_jcc(s, &c, get_field(s, m4));
> +    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
> +                        o->in1, o->in2);
> +    free_compare(&c);
> +    return DISAS_NEXT;
> +}


I realize that SELECT really is mostly identical to LOAD ON CONDITION,
except that we have a second input.

The following on top would unify both


diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def
index fb482b08b7..493f1d669c 100644
--- a/target/s390x/tcg/insn-data.def
+++ b/target/s390x/tcg/insn-data.def
@@ -781,8 +781,8 @@
 /* SEARCH STRING UNICODE */
     C(0xb9be, SRSTU,   RRE,   ETF3, 0, 0, 0, 0, srstu, 0)
 /* SELECT */
-    C(0xb9f0, SELR,    RRF_a, MIE3, r2, r3, new, r1_32, sel, 0)
-    C(0xb9e3, SELGR,   RRF_a, MIE3, r2, r3, r1, 0, sel, 0)
+    C(0xb9f0, SELR,    RRF_a, MIE3, r3, r2, new, r1_32, loc, 0)
+    C(0xb9e3, SELGR,   RRF_a, MIE3, r3, r2, r1, 0, loc, 0)
 
 /* SET ACCESS */
     C(0xb24e, SAR,     RRE,   Z,   0, r2_o, 0, 0, sar, 0)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index d5c536c60a..7805ffe879 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1528,16 +1528,6 @@ static DisasJumpType op_nxor(DisasContext *s, DisasOps 
*o)
     return DISAS_NEXT;
 }
 
-static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
-{
-    DisasCompare c;
-    disas_jcc(s, &c, get_field(s, m4));
-    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
-                        o->in1, o->in2);
-    free_compare(&c);
-    return DISAS_NEXT;
-}
-
 static DisasJumpType op_ni(DisasContext *s, DisasOps *o)
 {
     o->in1 = tcg_temp_new_i64();
@@ -2998,7 +2988,13 @@ static DisasJumpType op_loc(DisasContext *s, DisasOps *o)
 {
     DisasCompare c;
 
-    disas_jcc(s, &c, get_field(s, m3));
+    if (have_field(s, m3)) {
+        /* LOAD * ON CONDITION */
+        disas_jcc(s, &c, get_field(s, m3));
+    } else {
+        /* SELECT */
+        disas_jcc(s, &c, get_field(s, m4));
+    }
 
     if (c.is_64) {
         tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,


I can spot some advanced magic in op_loc for "!c.is64".

But even with that change, the SELECT test still crashes for me.

The problematic part is the last selfhrnz() test that makes QEMU crash.

This might be an existing BUG for op_loc already -- or in the TCG backend.

Maybe the disas_jcc/tcg_gen_movcond_i64 generates something unexpected on my 
machine?

-- 
Thanks,

David / dhildenb