Re: [PATCH v11 10/25] i386: move TCG cpu class initialization out of helper.c

[Richard Henderson]

On 12/11/20 2:31 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/i386/cpu.h             |  97 ++---------------------------
>  target/i386/tcg/helper-tcg.h  | 112 ++++++++++++++++++++++++++++++++++
>  target/i386/tcg/tcg-cpu.h     |  15 +++++
>  target/i386/cpu.c             |  33 ++++------
>  target/i386/helper.c          |  23 -------
>  target/i386/tcg/bpt_helper.c  |   1 +
>  target/i386/tcg/cc_helper.c   |   1 +
>  target/i386/tcg/excp_helper.c |   1 +
>  target/i386/tcg/fpu_helper.c  |  33 +++++-----
>  target/i386/tcg/int_helper.c  |   1 +
>  target/i386/tcg/mem_helper.c  |   1 +
>  target/i386/tcg/misc_helper.c |   1 +
>  target/i386/tcg/mpx_helper.c  |   1 +
>  target/i386/tcg/seg_helper.c  |   1 +
>  target/i386/tcg/smm_helper.c  |   2 +
>  target/i386/tcg/svm_helper.c  |   1 +
>  target/i386/tcg/tcg-cpu.c     |  71 +++++++++++++++++++++
>  target/i386/tcg/translate.c   |   1 +
>  target/i386/tcg/meson.build   |   1 +
>  19 files changed, 244 insertions(+), 153 deletions(-)
>  create mode 100644 target/i386/tcg/helper-tcg.h
>  create mode 100644 target/i386/tcg/tcg-cpu.h
>  create mode 100644 target/i386/tcg/tcg-cpu.c

This is doing a lot more than $SUBJECT, which afaict would just be the creation
of target/i386/tcg/tcg-cpu.c.

> +#define CC_DST  (env->cc_dst)
> +#define CC_SRC  (env->cc_src)
> +#define CC_SRC2 (env->cc_src2)
> +#define CC_OP   (env->cc_op)

Why are these moving within cpu.h?  If they move at all, they should be moved
out of here.  Better if they're simply removed -- this is a silly wrapping of
"env->foo" from ancient days.

> +/* float macros */
> +#define FT0    (env->ft0)
> +#define ST0    (env->fpregs[env->fpstt].d)
> +#define ST(n)  (env->fpregs[(env->fpstt + (n)) & 7].d)
> +#define ST1    ST(1)

Any chance these can move to fpu_helper.c?  Or, in the case of FT0, be 
eliminated?

> +/* cc_helper.c */
> +extern const uint8_t parity_table[256];

We should probably remove this and just use ctpop(x) & 1.

> +/*
> + * NOTE: the translator must set DisasContext.cc_op to CC_OP_EFLAGS
> + * after generating a call to a helper that uses this.
> + */
> +static inline void cpu_load_eflags(CPUX86State *env, int eflags,
> +                                   int update_mask)
> +{
> +    CC_SRC = eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> +    CC_OP = CC_OP_EFLAGS;
> +    env->df = 1 - (2 * ((eflags >> 10) & 1));
> +    env->eflags = (env->eflags & ~update_mask) |
> +        (eflags & update_mask) | 0x2;
> +}

This is complex enough I would be in favor of moving it out-of-line somewhere
convenient.

> +++ b/target/i386/tcg/tcg-cpu.h
> @@ -0,0 +1,15 @@
> +/*
> + * i386 TCG CPU class initialization
> + *
> + * Copyright 2020 SUSE LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef TCG_CPU_H
> +#define TCG_CPU_H
> +
> +void tcg_cpu_common_class_init(CPUClass *cc);
> +

Why does the new file need it's own header, with one declaration?


r~