[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] target/s390x: Fix R[NOX]SBG with T=1
From: |
Ilya Leoshkevich |
Subject: |
Re: [PATCH v2 1/2] target/s390x: Fix R[NOX]SBG with T=1 |
Date: |
Thu, 16 Mar 2023 18:15:06 +0100 |
User-agent: |
Evolution 3.46.4 (3.46.4-1.fc37) |
On Thu, 2023-03-16 at 09:41 +0100, Philippe Mathieu-Daudé wrote:
> On 16/3/23 00:56, Ilya Leoshkevich wrote:
> > RXSBG usage in the "filetests" test from the wasmtime testsuite
> > makes
> > tcg_reg_alloc_op() attempt to temp_load() a TEMP_VAL_DEAD
> > temporary,
> > causing an assertion failure:
> >
> > 0x01000a70: ec14 b040 3057 rxsbg %r1, %r4, 0xb0, 0x40,
> > 0x30
> >
> > OP after optimization and liveness analysis:
> > ---- 0000000001000a70 0000000000000004 0000000000000006
> > rotl_i64 tmp2,r4,$0x30 dead: 1 2
> > pref=0xffff
> > and_i64 tmp2,tmp2,$0x800000000000ffff dead: 1 pref=0xffff
> > [xor_i64 tmp3,tmp3,tmp2 dead: 1 2
> > pref=0xffff]
> > and_i64 cc_dst,tmp3,$0x800000000000ffff sync: 0 dead: 0 1
> > 2 pref=0xffff
> > mov_i64 psw_addr,$0x1000a76 sync: 0 dead: 0 1
> > pref=0xffff
> > mov_i32 cc_op,$0x6 sync: 0 dead: 0 1
> > pref=0xffff
> > call lookup_tb_ptr,$0x6,$1,tmp8,env dead: 1 pref=none
> > goto_ptr tmp8 dead: 0
> > set_label $L0
> > exit_tb $0x7fffe809d183
> >
> > ../tcg/tcg.c:3865: tcg fatal error
> >
> > The reason is that tmp3 does not have an initial value, which
> > confuses
> > the register allocator. This also affects the correctness of the
> > results.
> >
> > Fix by assigning R1 to it.
> >
> > Fixes: d6c6372e186e ("target-s390: Implement R[NOX]SBG")
>
> Exposed by 3ac6f91bca..dd161de75f?
Bisect points to:
commit e2e641fa3d5e730f128562d6901dcc729c9bf8a0
Author: Richard Henderson <richard.henderson@linaro.org>
Date: Sun Jan 29 14:09:00 2023 -1000
tcg: Change default temp lifetime to TEMP_TB
I will mention this.
> 3ac6f91bca target/s390x: Drop tcg_temp_free from translate.c
> dd161de75f target/s390x: Remove g_out, g_out2, g_in1, g_in2
>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> > target/s390x/tcg/translate.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/target/s390x/tcg/translate.c
> > b/target/s390x/tcg/translate.c
> > index 14c3896d529..6dd2f41ad08 100644
> > --- a/target/s390x/tcg/translate.c
> > +++ b/target/s390x/tcg/translate.c
> > @@ -3696,10 +3696,13 @@ static DisasJumpType op_rosbg(DisasContext
> > *s, DisasOps *o)
> > int i4 = get_field(s, i4);
> > int i5 = get_field(s, i5);
> > uint64_t mask;
> > + TCGv_i64 tmp;
> >
> > /* If this is a test-only form, arrange to discard the
> > result. */
> > if (i3 & 0x80) {
>
> tcg_debug_assert(o->out != NULL); ?
Ok, I will add this.
>
> > + tmp = o->out;
> > o->out = tcg_temp_new_i64();
> > + tcg_gen_mov_i64(o->out, tmp);
>
> Something bugs me with this pattern but I can't say why yet :(
Please let me know once you come up with something.
I will do s/tmp/orig_out/ send a v3 in the meantime.
> > }
> >
> > i3 &= 63;