Re: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower CPU QOM cast macro

[Thomas Huth]

On 30/01/2024 14.01, Igor Mammedov wrote:
> On Mon, 29 Jan 2024 17:44:56 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> > Mechanical patch produced running the command documented
> > in scripts/coccinelle/cpu_env.cocci_template header.
>
>
> commenting here since, I'm not expert on coccinelle scripts.
>
> On negative side we are permanently loosing type checking in this area.

Not really that much. Have a look at cpu_env(), it has a comment saying:

 "We validate that CPUArchState follows CPUState in cpu-all.h"

So instead of run-time checking, the check should have already been done 
during compile time, i.e. when you have a valid CPUState pointer, it should 
be possible to derive a valid CPUArchState pointer from it without much 
further checking during runtime.

> Is it worth it, what gains do we get with this series?

It's a small optimization, but why not?

> Side note,
> QOM cast expenses you are replacing could be negated by disabling
> CONFIG_QOM_CAST_DEBUG without killing type check code when it's enabled.
> That way you will speed up not only cpuenv access but also all other casts
> across the board.

Yes, but that checking is enabled by default and does not have such 
compile-time checks that could be used instead, so I think Philippe's series 
here is still a good idea.

> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> ...
> >   static inline void vmx_clear_nmi_blocking(CPUState *cpu)
> >   {
> > -    X86CPU *x86_cpu = X86_CPU(cpu);
> > -    CPUX86State *env = &x86_cpu->env;
> > -
> > -    env->hflags2 &= ~HF2_NMI_MASK;
>
> > +    cpu_env(cpu)->hflags2 &= ~HF2_NMI_MASK;
>
> this style of de-referencing return value of macro/function
> was discouraged in past and preferred way was 'Foo f = CAST(me); f->some_access
>
> (it's just imprint speaking, I don't recall where it comes from)

I agree, though the new code is perfectly valid, it looks nicer if we'd use 
a variable here instead.

 Thomas