Add a Coccinelle script to convert the following slow path
(due to the QOM cast macro):
&ARCH_CPU(..)->env
to the following fast path:
cpu_env(..)
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 1 +
scripts/coccinelle/cpu_env.cocci_template | 60 +++++++++++++++++++++++
2 files changed, 61 insertions(+)
create mode 100644 scripts/coccinelle/cpu_env.cocci_template
diff --git a/MAINTAINERS b/MAINTAINERS
index dfaca8323e..1d57130ff8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -157,6 +157,7 @@ F: accel/tcg/
F: accel/stubs/tcg-stub.c
F: util/cacheinfo.c
F: util/cacheflush.c
+F: scripts/coccinelle/cpu_env.cocci_template
F: scripts/decodetree.py
F: docs/devel/decodetree.rst
F: docs/devel/tcg*
diff --git a/scripts/coccinelle/cpu_env.cocci_template b/scripts/coccinelle/cpu_env.cocci_template
new file mode 100644
index 0000000000..53aa3a1fea
--- /dev/null
+++ b/scripts/coccinelle/cpu_env.cocci_template
@@ -0,0 +1,60 @@
+/*
+
+ Convert &ARCH_CPU(..)->env to use cpu_env(..).
+
+ Rationale: ARCH_CPU() might be slow, being a QOM cast macro.
+ cpu_env() is its fast equivalent.
+
+ SPDX-License-Identifier: GPL-2.0-or-later
+ SPDX-FileCopyrightText: Linaro Ltd 2024
+ SPDX-FileContributor: Philippe Mathieu-Daudé
+
+ Usage as of v8.2.0:
+
+ $ for targetdir in target/*; do test -d $targetdir || continue; \
+ export target=${targetdir:7}; \
+ sed \
+ -e "s/__CPUArchState__/$( \
+ git grep -h --no-line-number '@env: #CPU.*State' \
+ target/$target/cpu.h \
+ | sed -n -e 's/.*\(CPU.*State\).\?/\1/p')/g" \
+ -e "s/__ARCHCPU__/$( \
+ git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
+ target/$target/cpu-qom.h \
+ | sed -n -e 's/.*(\(.*\), .*, .*)/\1/p')/g" \
+ -e "s/__ARCH_CPU__/$( \
+ git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
+ target/$target/cpu-qom.h \
+ | sed -n -e 's/.*(.*, .*, \(.*\))/\1/p')/g" \
+ < scripts/coccinelle/cpu_env.cocci_template \
+ > $TMPDIR/cpu_env_$target.cocci; \
+ for dir in hw target/$target; do \
+ spatch --macro-file scripts/cocci-macro-file.h \
+ --sp-file $TMPDIR/cpu_env_$target.cocci \
+ --keep-comments \
+ --dir $dir \
+ --in-place; \
+ done; \
+ done
+
+*/
+
+@ CPUState_arg_used @
+CPUState *cs;
+identifier cpu;
+identifier env;
+@@
+- __ARCHCPU__ *cpu = __ARCH_CPU__(cs);
+- __CPUArchState__ *env = &cpu->env;
++ __CPUArchState__ *env = cpu_env(cs);
+ ... when != cpu
+
+@ depends on never CPUState_arg_used @
+identifier obj;
+identifier cpu;
+identifier env;
+@@
+- __ARCHCPU__ *cpu = __ARCH_CPU__(obj);
+- __CPUArchState__ *env = &cpu->env;
++ __CPUArchState__ *env = cpu_env(CPU(obj));
+ ... when != cpu
--
2.41.0
On 25/1/24 17:56, Philippe Mathieu-Daudé wrote:
> Add a Coccinelle script to convert the following slow path
> (due to the QOM cast macro):
>
> &ARCH_CPU(..)->env
>
> to the following fast path:
>
> cpu_env(..)
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS | 1 +
> scripts/coccinelle/cpu_env.cocci_template | 60 +++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
> create mode 100644 scripts/coccinelle/cpu_env.cocci_template
> diff --git a/scripts/coccinelle/cpu_env.cocci_template b/scripts/coccinelle/cpu_env.cocci_template
> new file mode 100644
> index 0000000000..53aa3a1fea
> --- /dev/null
> +++ b/scripts/coccinelle/cpu_env.cocci_template
> @@ -0,0 +1,60 @@
> +/*
> +
> + Convert &ARCH_CPU(..)->env to use cpu_env(..).
> +
> + Rationale: ARCH_CPU() might be slow, being a QOM cast macro.
> + cpu_env() is its fast equivalent.
> +
> + SPDX-License-Identifier: GPL-2.0-or-later
> + SPDX-FileCopyrightText: Linaro Ltd 2024
> + SPDX-FileContributor: Philippe Mathieu-Daudé
> +
> + Usage as of v8.2.0:
> +
> + $ for targetdir in target/*; do test -d $targetdir || continue; \
> + export target=${targetdir:7}; \
> + sed \
> + -e "s/__CPUArchState__/$( \
> + git grep -h --no-line-number '@env: #CPU.*State' \
> + target/$target/cpu.h \
> + | sed -n -e 's/.*\(CPU.*State\).\?/\1/p')/g" \
> + -e "s/__ARCHCPU__/$( \
> + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
> + target/$target/cpu-qom.h \
> + | sed -n -e 's/.*(\(.*\), .*, .*)/\1/p')/g" \
> + -e "s/__ARCH_CPU__/$( \
> + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
> + target/$target/cpu-qom.h \
> + | sed -n -e 's/.*(.*, .*, \(.*\))/\1/p')/g" \
> + < scripts/coccinelle/cpu_env.cocci_template \
> + > $TMPDIR/cpu_env_$target.cocci; \
> + for dir in hw target/$target; do \
> + spatch --macro-file scripts/cocci-macro-file.h \
> + --sp-file $TMPDIR/cpu_env_$target.cocci \
> + --keep-comments \
> + --dir $dir \
> + --in-place; \
> + done; \
> + done
> +
> +*/
> +
> +@ CPUState_arg_used @
> +CPUState *cs;
> +identifier cpu;
> +identifier env;
> +@@
> +- __ARCHCPU__ *cpu = __ARCH_CPU__(cs);
Here we remove ARCH_CPU(), ...
> +- __CPUArchState__ *env = &cpu->env;
> ++ __CPUArchState__ *env = cpu_env(cs);
> + ... when != cpu
> +
> +@ depends on never CPUState_arg_used @
> +identifier obj;
> +identifier cpu;
> +identifier env;
> +@@
> +- __ARCHCPU__ *cpu = __ARCH_CPU__(obj);
> +- __CPUArchState__ *env = &cpu->env;
> ++ __CPUArchState__ *env = cpu_env(CPU(obj));
... but here we just change it by a CPU() QOM call.
So this 2nd change is just style cleanup.
> + ... when != cpu
On Fri, Jan 26, 2024 at 11:38 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 25/1/24 17:56, Philippe Mathieu-Daudé wrote:
> > Add a Coccinelle script to convert the following slow path
> > (due to the QOM cast macro):
> >
> > &ARCH_CPU(..)->env
> >
> > to the following fast path:
> >
> > cpu_env(..)
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > MAINTAINERS | 1 +
> > scripts/coccinelle/cpu_env.cocci_template | 60 +++++++++++++++++++++++
> > 2 files changed, 61 insertions(+)
> > create mode 100644 scripts/coccinelle/cpu_env.cocci_template
>
>
> > diff --git a/scripts/coccinelle/cpu_env.cocci_template b/scripts/coccinelle/cpu_env.cocci_template
> > new file mode 100644
> > index 0000000000..53aa3a1fea
> > --- /dev/null
> > +++ b/scripts/coccinelle/cpu_env.cocci_template
> > @@ -0,0 +1,60 @@
> > +/*
> > +
> > + Convert &ARCH_CPU(..)->env to use cpu_env(..).
> > +
> > + Rationale: ARCH_CPU() might be slow, being a QOM cast macro.
> > + cpu_env() is its fast equivalent.
> > +
> > + SPDX-License-Identifier: GPL-2.0-or-later
> > + SPDX-FileCopyrightText: Linaro Ltd 2024
> > + SPDX-FileContributor: Philippe Mathieu-Daudé
> > +
> > + Usage as of v8.2.0:
> > +
> > + $ for targetdir in target/*; do test -d $targetdir || continue; \
> > + export target=${targetdir:7}; \
> > + sed \
> > + -e "s/__CPUArchState__/$( \
> > + git grep -h --no-line-number '@env: #CPU.*State' \
> > + target/$target/cpu.h \
> > + | sed -n -e 's/.*\(CPU.*State\).\?/\1/p')/g" \
> > + -e "s/__ARCHCPU__/$( \
> > + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
> > + target/$target/cpu-qom.h \
> > + | sed -n -e 's/.*(\(.*\), .*, .*)/\1/p')/g" \
> > + -e "s/__ARCH_CPU__/$( \
> > + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
> > + target/$target/cpu-qom.h \
> > + | sed -n -e 's/.*(.*, .*, \(.*\))/\1/p')/g" \
> > + < scripts/coccinelle/cpu_env.cocci_template \
> > + > $TMPDIR/cpu_env_$target.cocci; \
> > + for dir in hw target/$target; do \
> > + spatch --macro-file scripts/cocci-macro-file.h \
> > + --sp-file $TMPDIR/cpu_env_$target.cocci \
> > + --keep-comments \
> > + --dir $dir \
> > + --in-place; \
> > + done; \
> > + done
> > +
> > +*/
> > +
> > +@ CPUState_arg_used @
> > +CPUState *cs;
> > +identifier cpu;
> > +identifier env;
> > +@@
> > +- __ARCHCPU__ *cpu = __ARCH_CPU__(cs);
>
> Here we remove ARCH_CPU(), ...
>
> > +- __CPUArchState__ *env = &cpu->env;
> > ++ __CPUArchState__ *env = cpu_env(cs);
> > + ... when != cpu
> > +
> > +@ depends on never CPUState_arg_used @
> > +identifier obj;
> > +identifier cpu;
> > +identifier env;
> > +@@
> > +- __ARCHCPU__ *cpu = __ARCH_CPU__(obj);
> > +- __CPUArchState__ *env = &cpu->env;
> > ++ __CPUArchState__ *env = cpu_env(CPU(obj));
>
> ... but here we just change it by a CPU() QOM call.
> So this 2nd change is just style cleanup.
Can you also add a hunk that is
CPUState *cs;
@@
- CPU(cs)
+ cs
to clean up on the second? cpu_env(CPU(current_cpu)) is suboptimal
and also a bit ugly.
paolo
On 26/1/24 12:24, Paolo Bonzini wrote:
> On Fri, Jan 26, 2024 at 11:38 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 25/1/24 17:56, Philippe Mathieu-Daudé wrote:
>>> Add a Coccinelle script to convert the following slow path
>>> (due to the QOM cast macro):
>>>
>>> &ARCH_CPU(..)->env
>>>
>>> to the following fast path:
>>>
>>> cpu_env(..)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> MAINTAINERS | 1 +
>>> scripts/coccinelle/cpu_env.cocci_template | 60 +++++++++++++++++++++++
>>> 2 files changed, 61 insertions(+)
>>> create mode 100644 scripts/coccinelle/cpu_env.cocci_template
>>
>>
>>> diff --git a/scripts/coccinelle/cpu_env.cocci_template b/scripts/coccinelle/cpu_env.cocci_template
>>> new file mode 100644
>>> index 0000000000..53aa3a1fea
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/cpu_env.cocci_template
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> +
>>> + Convert &ARCH_CPU(..)->env to use cpu_env(..).
>>> +
>>> + Rationale: ARCH_CPU() might be slow, being a QOM cast macro.
>>> + cpu_env() is its fast equivalent.
>>> +
>>> + SPDX-License-Identifier: GPL-2.0-or-later
>>> + SPDX-FileCopyrightText: Linaro Ltd 2024
>>> + SPDX-FileContributor: Philippe Mathieu-Daudé
>>> +
>>> + Usage as of v8.2.0:
>>> +
>>> + $ for targetdir in target/*; do test -d $targetdir || continue; \
>>> + export target=${targetdir:7}; \
>>> + sed \
>>> + -e "s/__CPUArchState__/$( \
>>> + git grep -h --no-line-number '@env: #CPU.*State' \
>>> + target/$target/cpu.h \
>>> + | sed -n -e 's/.*\(CPU.*State\).\?/\1/p')/g" \
>>> + -e "s/__ARCHCPU__/$( \
>>> + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
>>> + target/$target/cpu-qom.h \
>>> + | sed -n -e 's/.*(\(.*\), .*, .*)/\1/p')/g" \
>>> + -e "s/__ARCH_CPU__/$( \
>>> + git grep -h --no-line-number OBJECT_DECLARE_CPU_TYPE.*CPU \
>>> + target/$target/cpu-qom.h \
>>> + | sed -n -e 's/.*(.*, .*, \(.*\))/\1/p')/g" \
>>> + < scripts/coccinelle/cpu_env.cocci_template \
>>> + > $TMPDIR/cpu_env_$target.cocci; \
>>> + for dir in hw target/$target; do \
>>> + spatch --macro-file scripts/cocci-macro-file.h \
>>> + --sp-file $TMPDIR/cpu_env_$target.cocci \
>>> + --keep-comments \
>>> + --dir $dir \
>>> + --in-place; \
>>> + done; \
>>> + done
>>> +
>>> +*/
>>> +
>>> +@ CPUState_arg_used @
>>> +CPUState *cs;
>>> +identifier cpu;
>>> +identifier env;
>>> +@@
>>> +- __ARCHCPU__ *cpu = __ARCH_CPU__(cs);
>>
>> Here we remove ARCH_CPU(), ...
>>
>>> +- __CPUArchState__ *env = &cpu->env;
>>> ++ __CPUArchState__ *env = cpu_env(cs);
>>> + ... when != cpu
>>> +
>>> +@ depends on never CPUState_arg_used @
>>> +identifier obj;
>>> +identifier cpu;
>>> +identifier env;
>>> +@@
>>> +- __ARCHCPU__ *cpu = __ARCH_CPU__(obj);
>>> +- __CPUArchState__ *env = &cpu->env;
>>> ++ __CPUArchState__ *env = cpu_env(CPU(obj));
>>
>> ... but here we just change it by a CPU() QOM call.
>> So this 2nd change is just style cleanup.
>
> Can you also add a hunk that is
>
> CPUState *cs;
> @@
> - CPU(cs)
> + cs
>
> to clean up on the second? cpu_env(CPU(current_cpu)) is suboptimal
> and also a bit ugly.
These case should be cleaned because this is already a CPUState*:
+ CPUX86State *env = cpu_env(CPU(current_cpu));
+ CPUPPCState *env = cpu_env(CPU(first_cpu));
But these (instance_init and QOM visitors) can't, the argument
isn't a CPUState*:
static void ev4_cpu_initfn(Object *obj)
{
- AlphaCPU *cpu = ALPHA_CPU(obj);
- CPUAlphaState *env = &cpu->env;
+ CPUAlphaState *env = cpu_env(CPU(obj));
@@ -5186,8 +5179,7 @@ static char *x86_cpuid_get_vendor(Object *obj,
Error **errp)
static void x86_cpuid_set_vendor(Object *obj, const char *value,
Error **errp)
{
- X86CPU *cpu = X86_CPU(obj);
- CPUX86State *env = &cpu->env;
+ CPUX86State *env = cpu_env(CPU(obj));
That said, these visitors take a Object* param because they implement
the generic QOM visitor API, but we know the visitor are registered
on classes/objects implementing CPUState, so QOM cast macro is
redundant.
On 26/1/24 13:34, Philippe Mathieu-Daudé wrote:
> On 26/1/24 12:24, Paolo Bonzini wrote:
>> On Fri, Jan 26, 2024 at 11:38 AM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> On 25/1/24 17:56, Philippe Mathieu-Daudé wrote:
>>>> Add a Coccinelle script to convert the following slow path
>>>> (due to the QOM cast macro):
>>>>
>>>> &ARCH_CPU(..)->env
>>>>
>>>> to the following fast path:
>>>>
>>>> cpu_env(..)
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> MAINTAINERS | 1 +
>>>> scripts/coccinelle/cpu_env.cocci_template | 60
>>>> +++++++++++++++++++++++
>>>> 2 files changed, 61 insertions(+)
>>>> create mode 100644 scripts/coccinelle/cpu_env.cocci_template
>>>
>>>
>>>> diff --git a/scripts/coccinelle/cpu_env.cocci_template
>>>> b/scripts/coccinelle/cpu_env.cocci_template
>>>> new file mode 100644
>>>> index 0000000000..53aa3a1fea
>>>> --- /dev/null
>>>> +++ b/scripts/coccinelle/cpu_env.cocci_template
>>>> @@ -0,0 +1,60 @@
>>>> +/*
>>>> +
>>>> + Convert &ARCH_CPU(..)->env to use cpu_env(..).
>>>> +
>>>> + Rationale: ARCH_CPU() might be slow, being a QOM cast macro.
>>>> + cpu_env() is its fast equivalent.
>>>> +
>>>> + SPDX-License-Identifier: GPL-2.0-or-later
>>>> + SPDX-FileCopyrightText: Linaro Ltd 2024
>>>> + SPDX-FileContributor: Philippe Mathieu-Daudé
>>>> +
>>>> + Usage as of v8.2.0:
>>>> +
>>>> + $ for targetdir in target/*; do test -d $targetdir || continue; \
>>>> + export target=${targetdir:7}; \
>>>> + sed \
>>>> + -e "s/__CPUArchState__/$( \
>>>> + git grep -h --no-line-number '@env: #CPU.*State' \
>>>> + target/$target/cpu.h \
>>>> + | sed -n -e 's/.*\(CPU.*State\).\?/\1/p')/g" \
>>>> + -e "s/__ARCHCPU__/$( \
>>>> + git grep -h --no-line-number
>>>> OBJECT_DECLARE_CPU_TYPE.*CPU \
>>>> + target/$target/cpu-qom.h \
>>>> + | sed -n -e 's/.*(\(.*\), .*, .*)/\1/p')/g" \
>>>> + -e "s/__ARCH_CPU__/$( \
>>>> + git grep -h --no-line-number
>>>> OBJECT_DECLARE_CPU_TYPE.*CPU \
>>>> + target/$target/cpu-qom.h \
>>>> + | sed -n -e 's/.*(.*, .*, \(.*\))/\1/p')/g" \
>>>> + < scripts/coccinelle/cpu_env.cocci_template \
>>>> + > $TMPDIR/cpu_env_$target.cocci; \
>>>> + for dir in hw target/$target; do \
>>>> + spatch --macro-file scripts/cocci-macro-file.h \
>>>> + --sp-file $TMPDIR/cpu_env_$target.cocci \
>>>> + --keep-comments \
>>>> + --dir $dir \
>>>> + --in-place; \
>>>> + done; \
>>>> + done
>>>> +
>>>> +*/
>>>> +
>>>> +@ CPUState_arg_used @
>>>> +CPUState *cs;
>>>> +identifier cpu;
>>>> +identifier env;
>>>> +@@
>>>> +- __ARCHCPU__ *cpu = __ARCH_CPU__(cs);
>>>
>>> Here we remove ARCH_CPU(), ...
>>>
>>>> +- __CPUArchState__ *env = &cpu->env;
>>>> ++ __CPUArchState__ *env = cpu_env(cs);
>>>> + ... when != cpu
>>>> +
>>>> +@ depends on never CPUState_arg_used @
>>>> +identifier obj;
>>>> +identifier cpu;
>>>> +identifier env;
>>>> +@@
>>>> +- __ARCHCPU__ *cpu = __ARCH_CPU__(obj);
>>>> +- __CPUArchState__ *env = &cpu->env;
>>>> ++ __CPUArchState__ *env = cpu_env(CPU(obj));
>>>
>>> ... but here we just change it by a CPU() QOM call.
>>> So this 2nd change is just style cleanup.
>>
>> Can you also add a hunk that is
>>
>> CPUState *cs;
>> @@
>> - CPU(cs)
>> + cs
>>
>> to clean up on the second? cpu_env(CPU(current_cpu)) is suboptimal
>> and also a bit ugly.
>
> These case should be cleaned because this is already a CPUState*:
>
> + CPUX86State *env = cpu_env(CPU(current_cpu));
>
> + CPUPPCState *env = cpu_env(CPU(first_cpu));
>
> But these (instance_init and QOM visitors) can't, the argument
> isn't a CPUState*:
>
> static void ev4_cpu_initfn(Object *obj)
> {
> - AlphaCPU *cpu = ALPHA_CPU(obj);
> - CPUAlphaState *env = &cpu->env;
> + CPUAlphaState *env = cpu_env(CPU(obj));
>
> @@ -5186,8 +5179,7 @@ static char *x86_cpuid_get_vendor(Object *obj,
> Error **errp)
> static void x86_cpuid_set_vendor(Object *obj, const char *value,
> Error **errp)
> {
> - X86CPU *cpu = X86_CPU(obj);
> - CPUX86State *env = &cpu->env;
> + CPUX86State *env = cpu_env(CPU(obj));
>
> That said, these visitors take a Object* param because they implement
> the generic QOM visitor API, but we know the visitor are registered
> on classes/objects implementing CPUState, so QOM cast macro is
> redundant.
Bah actually for CPU() this is not a problem, per commit 0d6d1ab499
("cpu: Avoid QOM casts for CPU()") you suggested :)
Keep the CPU() macro for a consistent developer experience and for
flexibility to exchange its implementation, but turn it into a pure,
unchecked C cast for now.
© 2016 - 2024 Red Hat, Inc.