Re: [PATCH v5 32/36] target/s390x: Use tcg_gen_atomic_cmpxchg_i128 for CDSG

[David Hildenbrand]

On 26.01.23 22:01, Richard Henderson wrote:
> On 1/26/23 01:27, David Hildenbrand wrote:
> > >    static DisasJumpType op_csst(DisasContext *s, DisasOps *o)
> > > @@ -5419,6 +5410,14 @@ static void prep_r1_P(DisasContext *s, DisasOps *o)
> > >    }
> > >    #define SPEC_prep_r1_P SPEC_r1_even
> > > +static void prep_r1_D64(DisasContext *s, DisasOps *o)
> > > +{
> > > +    int r1 = get_field(s, r1);
> > > +    o->out_128 = tcg_temp_new_i128();
> > > +    tcg_gen_concat_i64_i128(o->out_128, regs[r1 + 1], regs[r1]);
> >
> > I really wonder if we should simply move the tcg_gen_concat_i64_i128() into the 
> > op and use
> > a generic "allocate out_128" instead.
> >
> > At least that part here confused me heavily.
>
> Just the prep_r1_D64 bit of it?

Yes.

> Better for you as
>
>
>       C(0xeb3e, CDSG,    RSY_a, Z,   la2, r3_D64, 0, r1_D64, cdsg, 0)
>
>
>    static DisasJumpType op_cdsg(DisasContext *s, DisasOps *o)
>    {
>        int r1 = get_field(s, r1);
> +    o->out_128 = tcg_temp_new_i128();
> +    tcg_gen_concat_i64_i128(o->out_128, regs[r1 + 1], regs[r1]);
>
>
> The existing generic "allocate out_128" is named "new_x", which I thought was 
> also
> confusing, since this isn't an "x" format operation.  Since there's only one 
> use, I did it
> inline.

LGTM

--
Thanks,

David / dhildenb