Re: [PATCH 11/37] target/i386: validate SSE prefixes directly in the decoding table

[Richard Henderson]

On 9/12/22 00:03, Paolo Bonzini wrote:
> Many SSE and AVX instructions are only valid with specific prefixes
> (none, 66, F3, F2).  Introduce a direct way to encode this in the
> decoding table to avoid using decode groups too much.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/decode-new.c.inc | 37 ++++++++++++++++++++++++++++++++
>   target/i386/tcg/decode-new.h     |  1 +
>   2 files changed, 38 insertions(+)
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index f6c032c694..7b4fd9fb54 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -108,6 +108,22 @@
>   
>   #define avx2_256 .vex_special = X86_VEX_AVX2_256,
>   
> +#define P_00          1
> +#define P_66          (1 << PREFIX_DATA)
> +#define P_F3          (1 << PREFIX_REPZ)
> +#define P_F2          (1 << PREFIX_REPNZ)

These prefixes are already flags.  Do you really need to shift the shifted 
value?
I guess you need to choose a value for "no prefix", but I think you could also (ab,re)use 
PREFIX_LOCK or something...

> @@ -212,6 +212,7 @@ struct X86OpEntry {
>       X86CPUIDFeature cpuid : 8;
>       uint8_t      vex_class : 8;
>       X86VEXSpecial vex_special : 8;
> +    uint16_t     valid_prefix : 16;

Anyway, if you did, you'd only need 4 bits instead of 16.

That said, the logic is sound, and saving a few bits doesn't matter much.

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


r~