Re: [PATCH v2 1/2] target/s390x: Fix R[NOX]SBG with T=1

[Ilya Leoshkevich]

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;