1. Patchew
  2. QEMU
  3. View series

[PATCH] target/hppa: Speed up hppa_is_pa20()

Helge Deller posted 1 patch 10 months, 3 weeks ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH] target/hppa: Speed up hppa_is_pa20()
Posted by Helge Deller 10 months, 3 weeks ago 
Although the hppa_is_pa20() helper is costly due to string comparisms in
object_dynamic_cast(), it is called quite often during memory lookups
and at each start of a block of instruction translations.
Speed hppa_is_pa20() up by calling object_dynamic_cast() only once at
CPU creation and store the result in the is_pa20 of struct CPUArchState.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index a31dc32a9f..08ac1ec068 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -281,6 +281,7 @@ static TranslateFn *machine_HP_common_init_cpus(MachineState *machine)
     /* Create CPUs.  */
     for (unsigned int i = 0; i < smp_cpus; i++) {
         cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
+        cpu[i]->env.is_pa20 = object_dynamic_cast(OBJECT(cpu[i]), TYPE_HPPA64_CPU);
     }
 
     /*
diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index e45ba50a59..c37a701f44 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -208,6 +208,7 @@ typedef struct CPUArchState {
     uint64_t fr[32];
     uint64_t sr[8];          /* stored shifted into place for gva */
 
+    bool is_pa20;
     uint32_t psw;            /* All psw bits except the following:  */
     uint32_t psw_xb;         /* X and B, in their normal positions */
     target_ulong psw_n;      /* boolean */
@@ -294,7 +295,7 @@ struct HPPACPUClass {
 
 static inline bool hppa_is_pa20(CPUHPPAState *env)
 {
-    return object_dynamic_cast(OBJECT(env_cpu(env)), TYPE_HPPA64_CPU) != NULL;
+    return env->is_pa20;
 }
 
 static inline int HPPA_BTLB_ENTRIES(CPUHPPAState *env)
Re: [PATCH] target/hppa: Speed up hppa_is_pa20()
Posted by Richard Henderson 10 months, 3 weeks ago 
On 12/27/24 23:23, Helge Deller wrote:
> Although the hppa_is_pa20() helper is costly due to string comparisms in
> object_dynamic_cast(), it is called quite often during memory lookups
> and at each start of a block of instruction translations.
> Speed hppa_is_pa20() up by calling object_dynamic_cast() only once at
> CPU creation and store the result in the is_pa20 of struct CPUArchState.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index a31dc32a9f..08ac1ec068 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -281,6 +281,7 @@ static TranslateFn *machine_HP_common_init_cpus(MachineState *machine)
>       /* Create CPUs.  */
>       for (unsigned int i = 0; i < smp_cpus; i++) {
>           cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
> +        cpu[i]->env.is_pa20 = object_dynamic_cast(OBJECT(cpu[i]), TYPE_HPPA64_CPU);
>       }

This belongs in hppa_cpu_initfn, since it's internal to the workings of the cpu.
Otherwise,

> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
> index e45ba50a59..c37a701f44 100644
> --- a/target/hppa/cpu.h
> +++ b/target/hppa/cpu.h
> @@ -208,6 +208,7 @@ typedef struct CPUArchState {
>       uint64_t fr[32];
>       uint64_t sr[8];          /* stored shifted into place for gva */
>   
> +    bool is_pa20;

This placement will interact badly with your reset function.
Probably better to put it at the end of ArchCPU.


r~
Re: [PATCH] target/hppa: Speed up hppa_is_pa20()
Posted by BALATON Zoltan 10 months, 3 weeks ago 
On Sat, 28 Dec 2024, Helge Deller wrote:
> Although the hppa_is_pa20() helper is costly due to string comparisms in
> object_dynamic_cast(), it is called quite often during memory lookups
> and at each start of a block of instruction translations.
> Speed hppa_is_pa20() up by calling object_dynamic_cast() only once at
> CPU creation and store the result in the is_pa20 of struct CPUArchState.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index a31dc32a9f..08ac1ec068 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -281,6 +281,7 @@ static TranslateFn *machine_HP_common_init_cpus(MachineState *machine)
>     /* Create CPUs.  */
>     for (unsigned int i = 0; i < smp_cpus; i++) {
>         cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
> +        cpu[i]->env.is_pa20 = object_dynamic_cast(OBJECT(cpu[i]), TYPE_HPPA64_CPU);
>     }
>
>     /*
> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
> index e45ba50a59..c37a701f44 100644
> --- a/target/hppa/cpu.h
> +++ b/target/hppa/cpu.h
> @@ -208,6 +208,7 @@ typedef struct CPUArchState {
>     uint64_t fr[32];
>     uint64_t sr[8];          /* stored shifted into place for gva */
>
> +    bool is_pa20;
>     uint32_t psw;            /* All psw bits except the following:  */
>     uint32_t psw_xb;         /* X and B, in their normal positions */
>     target_ulong psw_n;      /* boolean */
> @@ -294,7 +295,7 @@ struct HPPACPUClass {
>
> static inline bool hppa_is_pa20(CPUHPPAState *env)
> {
> -    return object_dynamic_cast(OBJECT(env_cpu(env)), TYPE_HPPA64_CPU) != NULL;
> +    return env->is_pa20;
> }

Now this function name is longer than what it extends to so maybe it would 
be simpler to drop the inline function and use env->is_pa20 directly 
where it's needed? Is there a reason to keep the function?

Regards,
BALATON Zoltan

>
> static inline int HPPA_BTLB_ENTRIES(CPUHPPAState *env)
>
>
Re: [PATCH] target/hppa: Speed up hppa_is_pa20()
Posted by Helge Deller 10 months, 3 weeks ago 
On 12/28/24 12:16, BALATON Zoltan wrote:
> On Sat, 28 Dec 2024, Helge Deller wrote:
>> Although the hppa_is_pa20() helper is costly due to string comparisms in
>> object_dynamic_cast(), it is called quite often during memory lookups
>> and at each start of a block of instruction translations.
>> Speed hppa_is_pa20() up by calling object_dynamic_cast() only once at
>> CPU creation and store the result in the is_pa20 of struct CPUArchState.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index a31dc32a9f..08ac1ec068 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -281,6 +281,7 @@ static TranslateFn *machine_HP_common_init_cpus(MachineState *machine)
>>     /* Create CPUs.  */
>>     for (unsigned int i = 0; i < smp_cpus; i++) {
>>         cpu[i] = HPPA_CPU(cpu_create(machine->cpu_type));
>> +        cpu[i]->env.is_pa20 = object_dynamic_cast(OBJECT(cpu[i]), TYPE_HPPA64_CPU);
>>     }
>>
>>     /*
>> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
>> index e45ba50a59..c37a701f44 100644
>> --- a/target/hppa/cpu.h
>> +++ b/target/hppa/cpu.h
>> @@ -208,6 +208,7 @@ typedef struct CPUArchState {
>>     uint64_t fr[32];
>>     uint64_t sr[8];          /* stored shifted into place for gva */
>>
>> +    bool is_pa20;
>>     uint32_t psw;            /* All psw bits except the following:  */
>>     uint32_t psw_xb;         /* X and B, in their normal positions */
>>     target_ulong psw_n;      /* boolean */
>> @@ -294,7 +295,7 @@ struct HPPACPUClass {
>>
>> static inline bool hppa_is_pa20(CPUHPPAState *env)
>> {
>> -    return object_dynamic_cast(OBJECT(env_cpu(env)), TYPE_HPPA64_CPU) != NULL;
>> +    return env->is_pa20;
>> }
>
> Now this function name is longer than what it extends to so maybe it
> would be simpler to drop the inline function and use env->is_pa20
> directly where it's needed?

Yes, that's a possible cleanup which can be done afterwards.

> Is there a reason to keep the function?

Personally I like it more than the "env->is_pa20".
Richard, any opinion from your side? Should I send a such a replacement patch?

Helge
Re: [PATCH] target/hppa: Speed up hppa_is_pa20()
Posted by Richard Henderson 10 months, 3 weeks ago 
On 12/28/24 13:12, Helge Deller wrote:
> On 12/28/24 12:16, BALATON Zoltan wrote:
>>> static inline bool hppa_is_pa20(CPUHPPAState *env)
>>> {
>>> -    return object_dynamic_cast(OBJECT(env_cpu(env)), TYPE_HPPA64_CPU) != NULL;
>>> +    return env->is_pa20;
>>> }
>>
>> Now this function name is longer than what it extends to so maybe it
>> would be simpler to drop the inline function and use env->is_pa20
>> directly where it's needed?
> 
> Yes, that's a possible cleanup which can be done afterwards.
> 
>> Is there a reason to keep the function?
> 
> Personally I like it more than the "env->is_pa20".
> Richard, any opinion from your side? Should I send a such a replacement patch?

I like keeping the accessor function.


r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.