Re: [PATCH 42/67] target/arm: Convert handle_rev to decodetree

[Richard Henderson]

On 12/3/24 05:49, Peter Maydell wrote:
> On Sun, 1 Dec 2024 at 15:16, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > This includes REV16, REV32, REV64.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
>
> > @@ -10070,10 +10003,6 @@ static void disas_simd_two_reg_misc(DisasContext *s, 
> > uint32_t insn)
> >       TCGv_ptr tcg_fpstatus;
> >
> >       switch (opcode) {
> > -    case 0x0: /* REV64, REV32 */
> > -    case 0x1: /* REV16 */
> > -        handle_rev(s, opcode, u, is_q, size, rn, rd);
> > -        return;
> >       case 0x12: /* XTN, XTN2, SQXTUN, SQXTUN2 */
> >       case 0x14: /* SQXTN, SQXTN2, UQXTN, UQXTN2 */
> >           if (size == 3) {
> > @@ -10276,6 +10205,8 @@ static void disas_simd_two_reg_misc(DisasContext *s, 
> > uint32_t insn)
> >           break;
> >       }
> >       default:
> > +    case 0x0: /* REV64 */
> > +    case 0x1: /* REV16, REV32 */
>
> REV32 is case 0x0, not 0x1, per the comments deleted above.
>
> >       case 0x3: /* SUQADD, USQADD */
> >       case 0x4: /* CLS, CLZ */
> >       case 0x5: /* CNT, NOT, RBIT */
> > diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> > index 4f8231d07a..2531809096 100644
> > --- a/target/arm/tcg/a64.decode
> > +++ b/target/arm/tcg/a64.decode
> > @@ -73,6 +73,7 @@
> >
> >   @qrr_b          . q:1 ...... .. ...... ...... rn:5 rd:5  &qrr_e esz=0
> >   @qrr_h          . q:1 ...... .. ...... ...... rn:5 rd:5  &qrr_e esz=1
> > +@qrr_bh         . q:1 ...... . esz:1 ...... ...... rn:5 rd:5  &qrr_e
> >   @qrr_e          . q:1 ...... esz:2 ...... ...... rn:5 rd:5  &qrr_e
> >
> >   @qrrr_b         . q:1 ...... ... rm:5 ...... rn:5 rd:5  &qrrr_e esz=0
> > @@ -1657,3 +1658,7 @@ CMGE0_v         0.10 1110 ..1 00000 10001 0 ..... .....   
> >   @qrr_e
> >   CMEQ0_v         0.00 1110 ..1 00000 10011 0 ..... .....     @qrr_e
> >   CMLE0_v         0.10 1110 ..1 00000 10011 0 ..... .....     @qrr_e
> >   CMLT0_v         0.00 1110 ..1 00000 10101 0 ..... .....     @qrr_e
> > +
> > +REV16_v         0.00 1110 001 00000 00011 0 ..... .....     @qrr_b
> > +REV32_v         0.10 1110 0.1 00000 00011 0 ..... .....     @qrr_bh
> > +REV64_v         0.00 1110 ..1 00000 00001 0 ..... .....     @qrr_e
>
> This doesn't look quite right -- in the decode table in C4.1.96.21,
> 2-reg misc REV32 is opcode 00000, like REV64, not 00001 like REV16.
>
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -1660,5 +1660,5 @@ CMLE0_v         0.10 1110 ..1 00000 10011 0
> ..... .....     @qrr_e
>   CMLT0_v         0.00 1110 ..1 00000 10101 0 ..... .....     @qrr_e
>
>   REV16_v         0.00 1110 001 00000 00011 0 ..... .....     @qrr_b
> -REV32_v         0.10 1110 0.1 00000 00011 0 ..... .....     @qrr_bh
> +REV32_v         0.10 1110 0.1 00000 00001 0 ..... .....     @qrr_bh
>   REV64_v         0.00 1110 ..1 00000 00001 0 ..... .....     @qrr_e
>
> should I think be the right fixup.

Yep, my bad.  I remember "fixing" the comment, because I thought it was off.  I must have 
been tired, or something.  Odd that this passes risu... maybe we're missing a pattern.


r~