[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Thomas Huth
Subject: Re: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower CPU QOM cast macro
Date: Tue, 12 Mar 2024 12:24:49 +0100
User-agent: Mozilla Thunderbird

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]