| Message ID | 20240110224408.10444-16-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Introduce TCG_COND_TST{EQ,NE} | expand |
Hi Richard, On 10/1/24 23:43, Richard Henderson wrote: > Avoid code duplication by handling 7 of the 14 cases > by inverting the test for the other 7 cases. > > Use TCG_COND_TSTNE for cc in {1,3}. > Use (cc - 1) <= 1 for cc in {1,2}. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/s390x/tcg/translate.c | 82 +++++++++++++----------------------- > 1 file changed, 30 insertions(+), 52 deletions(-) > > diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c > index ae4e7b27ec..168974f2e6 100644 > --- a/target/s390x/tcg/translate.c > +++ b/target/s390x/tcg/translate.c > @@ -885,67 +885,45 @@ static void disas_jcc(DisasContext *s, DisasCompare *c, uint32_t mask) > case CC_OP_STATIC: > c->is_64 = false; > c->u.s32.a = cc_op; > - switch (mask) { > - case 0x8 | 0x4 | 0x2: /* cc != 3 */ > - cond = TCG_COND_NE; > + > + /* Fold half of the cases using bit 3 to invert. */ > + switch (mask & 8 ? mask ^ 0xf : mask) { > + case 0x1: /* cc == 3 */ > + cond = TCG_COND_EQ; > c->u.s32.b = tcg_constant_i32(3); > break; > - case 0x8 | 0x4 | 0x1: /* cc != 2 */ > - cond = TCG_COND_NE; > - c->u.s32.b = tcg_constant_i32(2); > - break; > - case 0x8 | 0x2 | 0x1: /* cc != 1 */ > - cond = TCG_COND_NE; > - c->u.s32.b = tcg_constant_i32(1); > - break; > - case 0x8 | 0x2: /* cc == 0 || cc == 2 => (cc & 1) == 0 */ > - cond = TCG_COND_EQ; > - c->u.s32.a = tcg_temp_new_i32(); > - c->u.s32.b = tcg_constant_i32(0); > - tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); > - break; > - case 0x8 | 0x4: /* cc < 2 */ > - cond = TCG_COND_LTU; > - c->u.s32.b = tcg_constant_i32(2); > - break; > - case 0x8: /* cc == 0 */ > - cond = TCG_COND_EQ; > - c->u.s32.b = tcg_constant_i32(0); > - break; > - case 0x4 | 0x2 | 0x1: /* cc != 0 */ > - cond = TCG_COND_NE; > - c->u.s32.b = tcg_constant_i32(0); > - break; > - case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ > - cond = TCG_COND_NE; > - c->u.s32.a = tcg_temp_new_i32(); > - c->u.s32.b = tcg_constant_i32(0); > - tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); > - break; > - case 0x4: /* cc == 1 */ > - cond = TCG_COND_EQ; > - c->u.s32.b = tcg_constant_i32(1); > - break; > - case 0x2 | 0x1: /* cc > 1 */ > - cond = TCG_COND_GTU; > - c->u.s32.b = tcg_constant_i32(1); > - break; > case 0x2: /* cc == 2 */ > cond = TCG_COND_EQ; > c->u.s32.b = tcg_constant_i32(2); > break; > - case 0x1: /* cc == 3 */ > + case 0x4: /* cc == 1 */ > cond = TCG_COND_EQ; > - c->u.s32.b = tcg_constant_i32(3); > + c->u.s32.b = tcg_constant_i32(1); > + break; > + case 0x2 | 0x1: /* cc == 2 || cc == 3 => cc > 1 */ > + cond = TCG_COND_GTU; > + c->u.s32.b = tcg_constant_i32(1); > + break; > + case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ > + cond = TCG_COND_TSTNE; > + c->u.s32.b = tcg_constant_i32(1); Don't we need to AND? c->u.s32.a = tcg_temp_new_i32(); tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); > + break; > + case 0x4 | 0x2: /* cc == 1 || cc == 2 => (cc - 1) <= 1 */ > + cond = TCG_COND_LEU; > + c->u.s32.a = tcg_temp_new_i32(); > + c->u.s32.b = tcg_constant_i32(1); > + tcg_gen_addi_i32(c->u.s32.a, cc_op, -1); > + break; > + case 0x4 | 0x2 | 0x1: /* cc != 0 */ > + cond = TCG_COND_NE; > + c->u.s32.b = tcg_constant_i32(0); > break; > default: > - /* CC is masked by something else: (8 >> cc) & mask. */ > - cond = TCG_COND_NE; > - c->u.s32.a = tcg_temp_new_i32(); > - c->u.s32.b = tcg_constant_i32(0); > - tcg_gen_shr_i32(c->u.s32.a, tcg_constant_i32(8), cc_op); > - tcg_gen_andi_i32(c->u.s32.a, c->u.s32.a, mask); > - break; > + /* case 0: never, handled above. */ > + g_assert_not_reached(); > + } > + if (mask & 8) { > + cond = tcg_invert_cond(cond); > } > break; >
On 1/17/24 09:19, Philippe Mathieu-Daudé wrote: >> + case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ >> + cond = TCG_COND_TSTNE; >> + c->u.s32.b = tcg_constant_i32(1); > > Don't we need to AND? > > c->u.s32.a = tcg_temp_new_i32(); > tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); No, that's the TSTNE cond there. r~
On 17/1/24 04:19, Richard Henderson wrote: > On 1/17/24 09:19, Philippe Mathieu-Daudé wrote: >>> + case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ >>> + cond = TCG_COND_TSTNE; >>> + c->u.s32.b = tcg_constant_i32(1); >> >> Don't we need to AND? >> >> c->u.s32.a = tcg_temp_new_i32(); >> tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); > > No, that's the TSTNE cond there. This patch as is was too complex for me so I split it in very dumb steps to get a trivial patch: https://lore.kernel.org/qemu-devel/20240119232302.50393-6-philmd@linaro.org/. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index ae4e7b27ec..168974f2e6 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -885,67 +885,45 @@ static void disas_jcc(DisasContext *s, DisasCompare *c, uint32_t mask) case CC_OP_STATIC: c->is_64 = false; c->u.s32.a = cc_op; - switch (mask) { - case 0x8 | 0x4 | 0x2: /* cc != 3 */ - cond = TCG_COND_NE; + + /* Fold half of the cases using bit 3 to invert. */ + switch (mask & 8 ? mask ^ 0xf : mask) { + case 0x1: /* cc == 3 */ + cond = TCG_COND_EQ; c->u.s32.b = tcg_constant_i32(3); break; - case 0x8 | 0x4 | 0x1: /* cc != 2 */ - cond = TCG_COND_NE; - c->u.s32.b = tcg_constant_i32(2); - break; - case 0x8 | 0x2 | 0x1: /* cc != 1 */ - cond = TCG_COND_NE; - c->u.s32.b = tcg_constant_i32(1); - break; - case 0x8 | 0x2: /* cc == 0 || cc == 2 => (cc & 1) == 0 */ - cond = TCG_COND_EQ; - c->u.s32.a = tcg_temp_new_i32(); - c->u.s32.b = tcg_constant_i32(0); - tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); - break; - case 0x8 | 0x4: /* cc < 2 */ - cond = TCG_COND_LTU; - c->u.s32.b = tcg_constant_i32(2); - break; - case 0x8: /* cc == 0 */ - cond = TCG_COND_EQ; - c->u.s32.b = tcg_constant_i32(0); - break; - case 0x4 | 0x2 | 0x1: /* cc != 0 */ - cond = TCG_COND_NE; - c->u.s32.b = tcg_constant_i32(0); - break; - case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ - cond = TCG_COND_NE; - c->u.s32.a = tcg_temp_new_i32(); - c->u.s32.b = tcg_constant_i32(0); - tcg_gen_andi_i32(c->u.s32.a, cc_op, 1); - break; - case 0x4: /* cc == 1 */ - cond = TCG_COND_EQ; - c->u.s32.b = tcg_constant_i32(1); - break; - case 0x2 | 0x1: /* cc > 1 */ - cond = TCG_COND_GTU; - c->u.s32.b = tcg_constant_i32(1); - break; case 0x2: /* cc == 2 */ cond = TCG_COND_EQ; c->u.s32.b = tcg_constant_i32(2); break; - case 0x1: /* cc == 3 */ + case 0x4: /* cc == 1 */ cond = TCG_COND_EQ; - c->u.s32.b = tcg_constant_i32(3); + c->u.s32.b = tcg_constant_i32(1); + break; + case 0x2 | 0x1: /* cc == 2 || cc == 3 => cc > 1 */ + cond = TCG_COND_GTU; + c->u.s32.b = tcg_constant_i32(1); + break; + case 0x4 | 0x1: /* cc == 1 || cc == 3 => (cc & 1) != 0 */ + cond = TCG_COND_TSTNE; + c->u.s32.b = tcg_constant_i32(1); + break; + case 0x4 | 0x2: /* cc == 1 || cc == 2 => (cc - 1) <= 1 */ + cond = TCG_COND_LEU; + c->u.s32.a = tcg_temp_new_i32(); + c->u.s32.b = tcg_constant_i32(1); + tcg_gen_addi_i32(c->u.s32.a, cc_op, -1); + break; + case 0x4 | 0x2 | 0x1: /* cc != 0 */ + cond = TCG_COND_NE; + c->u.s32.b = tcg_constant_i32(0); break; default: - /* CC is masked by something else: (8 >> cc) & mask. */ - cond = TCG_COND_NE; - c->u.s32.a = tcg_temp_new_i32(); - c->u.s32.b = tcg_constant_i32(0); - tcg_gen_shr_i32(c->u.s32.a, tcg_constant_i32(8), cc_op); - tcg_gen_andi_i32(c->u.s32.a, c->u.s32.a, mask); - break; + /* case 0: never, handled above. */ + g_assert_not_reached(); + } + if (mask & 8) { + cond = tcg_invert_cond(cond); } break;
Avoid code duplication by handling 7 of the 14 cases by inverting the test for the other 7 cases. Use TCG_COND_TSTNE for cc in {1,3}. Use (cc - 1) <= 1 for cc in {1,2}. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/tcg/translate.c | 82 +++++++++++++----------------------- 1 file changed, 30 insertions(+), 52 deletions(-)