Re: [Qemu-devel] [RFC PATCH v1 05/22] target/i386: introduce gen_ld_modrm_* helpers

[Richard Henderson]

On 7/31/19 10:56 AM, Jan Bobek wrote:
> These help with decoding/loading ModR/M vector operands; the operand's
> register offset is returned, which is suitable for use with gvec
> infrastructure.
> 
> Signed-off-by: Jan Bobek <address@hidden>
> ---
>  target/i386/translate.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 9e22eca2dc..7548677e1f 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -3040,6 +3040,53 @@ static const struct SSEOpHelper_eppi 
> sse_op_table7[256] = {
>      [0xdf] = AESNI_OP(aeskeygenassist),
>  };
>  
> +static inline void gen_ld_modrm_PqQq(CPUX86State *env, DisasContext *s, int 
> modrm,
> +                                     uint32_t* dofs, uint32_t* aofs)

s/uint32_t* /uint32_t */

Drop the inlines; let the compiler choose.


> +{
> +    const int mod = (modrm >> 6) & 3;
> +    const int reg = (modrm >> 3) & 7; /* no REX_R */
> +    *dofs = offsetof(CPUX86State, fpregs[reg].mmx);
> +
> +    if(mod == 3) {

s/if(/if (/

Both of these errors should be caught by ./scripts/checkpatch.pl.

> +        gen_ldo_env_A0(s, *aofs); /* FIXME this needs to load 32 bytes for 
> YMM 

Better as "TODO", since this isn't broken and in need of fixing, since we do
not yet support AVX.

Otherwise,
Reviewed-by: Richard Henderson <address@hidden>


r~