Re: [PATCH v2 9/9] target/arm: Convert Neon one-register-and-immediate insns to decodetree

[Richard Henderson]

On 5/22/20 7:55 AM, Peter Maydell wrote:
> Convert the insns in the one-register-and-immediate group to decodetree.
> 
> In the new decode, our asimd_imm_const() function returns a 64-bit value
> rather than a 32-bit one, which means we don't need to treat cmode=14 op=1
> as a special case in the decoder (it is the only encoding where the two
> halves of the 64-bit value are different).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/neon-dp.decode       |  22 ++++++
>  target/arm/translate-neon.inc.c | 118 ++++++++++++++++++++++++++++++++
>  target/arm/translate.c          | 101 +--------------------------
>  3 files changed, 142 insertions(+), 99 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

because this is a faithful transliteration of the existing code, but...

> +    switch (cmode) {
> +    case 0: case 1:
> +        /* no-op */
> +        break;
> +    case 2: case 3:
> +        imm <<= 8;
> +        break;
> +    case 4: case 5:
> +        imm <<= 16;
> +        break;
> +    case 6: case 7:
> +        imm <<= 24;
> +        break;
> +    case 8: case 9:
> +        imm |= imm << 16;
> +        break;
> +    case 10: case 11:
> +        imm = (imm << 8) | (imm << 24);
> +        break;

It might be clearer to use dup_const for each case, which would more closely
match the pseudocode.  E.g. here,

    return dup_const(MO_16, imm << 8);

> +        imm |= (imm << 8) | (imm << 16) | (imm << 24);

    return dup_const(MO_8, imm);

Something to remember for a follow-up.

r~