Re: [Qemu-devel] [PATCH 03/10] target-arm: A64: Add decode skeleton for SIMD data processing insns

[Richard Henderson]

On 01/10/2014 09:12 AM, Peter Maydell wrote:
> +static inline AArch64DecodeFn *lookup_disas_fn(AArch64DecodeTable *table,
> +                                               uint32_t insn)

Better make table const.

> +static AArch64DecodeTable data_proc_simd[] = {

So that you can make this const.

> +/* C3.6.1 EXT
> + *   31  30 29         24 23 22  21 20  16 15  14  11 10  9    5 4    0
> + * +---+---+-------------+-----+---+------+---+------+---+------+------+
> + * | 0 | Q | 0 0 1 1 1 0 | op2 | 0 |  Rm  | 0 | imm4 | 0 |  Rn  |  Rd  |
> + * +---+---+-------------+-----+---+------+---+------+---+------+------+
> + */

Error...        1

> +/* C3.6.16 AdvSIMD three same
> + *  31 30  29 28       24 23  22  21 20  16 15    11  10 9    5 4    0
> + * +-----+---+-----------+------+---+------+--------+---+------+------+
> + * | 0 1 | U | 1 1 1 1 0 | size | 1 |  Rm  | opcode | 1 |  Rn  |  Rd  |
> + * +-----+---+-----------+------+---+------+--------+---+------+------+
> + */

Error.  Cut and paste?

> +    /* pattern  ,  mask     ,  fn                        */
> +    { 0x0e200400, 0x9f200400, disas_simd_three_reg_same },           ok
> +    { 0x0e200000, 0x9f200c00, disas_simd_three_reg_diff },           ok
> +    { 0x0e200800, 0x9f3e0c00, disas_simd_two_reg_misc },             ok
> +    { 0x0e300800, 0x9f3e0c00, disas_simd_across_lanes },             ok
> +    { 0x0e000400, 0x9fe08400, disas_simd_copy },                     ok
> +    { 0x0f000000, 0x9f000400, disas_simd_indexed_vector },           ok
> +    /* simd_mod_imm decode is a subset of simd_shift_imm, so must precede it 
> */
> +    { 0x0f000400, 0x9ff80400, disas_simd_mod_imm },                  ok
> +    { 0x0f000400, 0x9f800400, disas_simd_shift_imm },                ok
> +    { 0x0e000000, 0xbf208c00, disas_simd_tb },                       ok
> +    { 0x0e000800, 0xbf208c00, disas_simd_zip_trn },                  ok
> +    { 0x2e000000, 0xbf208400, disas_simd_ext },                      ok
> +    { 0x5e200400, 0xdf200400, disas_simd_scalar_three_reg_same },    ok
> +    { 0x5e200000, 0xdf200c00, disas_simd_scalar_three_reg_diff },    ok
> +    { 0x5e200800, 0xdf3e0c00, disas_simd_scalar_two_reg_misc },      ok
> +    { 0x5e300800, 0xdf3e0c00, disas_simd_scalar_pairwise },          ok
> +    { 0x5e000400, 0xdfe08400, disas_simd_scalar_copy },              ok
> +    { 0x5f000000, 0xdf000400, disas_simd_scalar_indexed },           ok
> +    { 0x5f000400, 0xdf800400, disas_simd_scalar_shift_imm },         ok
> +    { 0x4e280800, 0xff3e0c00, disas_crypto_aes },                    ok
> +    { 0x5e000000, 0xff208c00, disas_crypto_three_reg_sha },          ok
> +    { 0x5e280800, 0xff3e0c00, disas_crypto_two_reg_sha },            ok
> +    { 0x00000000, 0x00000000, NULL }

The errors in the comments above are not present in this table.  I've verified
the pattern and mask entries, but not the ordering requirements.

> +        (fn) (s, insn);

Surely coding style sez

    fn(s, insn);
or
    (*fn)(s, insn);

Otherwise,

Reviewed-by: Richard Henderson <address@hidden>


r~