| Message ID | 20231009110239.66778-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target: Use env_archcpu() instead of ARCH_CPU(env_cpu(env)) | expand |
On 10/9/23 08:02, Philippe Mathieu-Daudé wrote: > When CPUArchState* is available (here CPURISCVState*), we > can use the fast env_archcpu() macro to get ArchCPU* (here > RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower > when building with --enable-qom-cast-debug. > > Inspired-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > target/riscv/internals.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/internals.h b/target/riscv/internals.h > index b5f823c7ec..8239ae83cc 100644 > --- a/target/riscv/internals.h > +++ b/target/riscv/internals.h > @@ -87,7 +87,7 @@ enum { > static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int32_t)f; > } else { > return f | MAKE_64BIT_MASK(32, 32); > @@ -97,7 +97,7 @@ static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > { > /* Disable NaN-boxing check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint32_t)f; > } > > @@ -113,7 +113,7 @@ static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int16_t)f; > } else { > return f | MAKE_64BIT_MASK(16, 48); > @@ -123,7 +123,7 @@ static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f) > { > /* Disable nanbox check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint16_t)f; > } >
On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: > When CPUArchState* is available (here CPURISCVState*), we > can use the fast env_archcpu() macro to get ArchCPU* (here > RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower > when building with --enable-qom-cast-debug. > > Inspired-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> By the way, does the community has the plan to support heterogeneous architecture cpus in one soc? If so, maybe we have to do this qom cast somewhere. Zhiwei > --- > target/riscv/internals.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/internals.h b/target/riscv/internals.h > index b5f823c7ec..8239ae83cc 100644 > --- a/target/riscv/internals.h > +++ b/target/riscv/internals.h > @@ -87,7 +87,7 @@ enum { > static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int32_t)f; > } else { > return f | MAKE_64BIT_MASK(32, 32); > @@ -97,7 +97,7 @@ static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > { > /* Disable NaN-boxing check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint32_t)f; > } > > @@ -113,7 +113,7 @@ static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int16_t)f; > } else { > return f | MAKE_64BIT_MASK(16, 48); > @@ -123,7 +123,7 @@ static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f) > { > /* Disable nanbox check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint16_t)f; > } >
On Mon, Oct 09, 2023 at 01:02:35PM +0200, Philippe Mathieu-Daudé wrote: > When CPUArchState* is available (here CPURISCVState*), we > can use the fast env_archcpu() macro to get ArchCPU* (here > RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower > when building with --enable-qom-cast-debug. > > Inspired-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > target/riscv/internals.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/internals.h b/target/riscv/internals.h > index b5f823c7ec..8239ae83cc 100644 > --- a/target/riscv/internals.h > +++ b/target/riscv/internals.h > @@ -87,7 +87,7 @@ enum { > static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int32_t)f; > } else { > return f | MAKE_64BIT_MASK(32, 32); > @@ -97,7 +97,7 @@ static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > { > /* Disable NaN-boxing check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint32_t)f; > } > > @@ -113,7 +113,7 @@ static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int16_t)f; > } else { > return f | MAKE_64BIT_MASK(16, 48); > @@ -123,7 +123,7 @@ static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f) > { > /* Disable nanbox check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint16_t)f; > } Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
On 10/9/23 05:42, LIU Zhiwei wrote: > > On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >> When CPUArchState* is available (here CPURISCVState*), we >> can use the fast env_archcpu() macro to get ArchCPU* (here >> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >> when building with --enable-qom-cast-debug. >> >> Inspired-by: Richard W.M. Jones <rjones@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > > By the way, does the community has the plan to support heterogeneous architecture cpus in > one soc? Yes. > If so, maybe we have to do this qom cast somewhere. No, I don't think so. Or at least not in these places. r~
On Mon, Oct 9, 2023 at 9:03 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > When CPUArchState* is available (here CPURISCVState*), we > can use the fast env_archcpu() macro to get ArchCPU* (here > RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower > when building with --enable-qom-cast-debug. > > Inspired-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/internals.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/internals.h b/target/riscv/internals.h > index b5f823c7ec..8239ae83cc 100644 > --- a/target/riscv/internals.h > +++ b/target/riscv/internals.h > @@ -87,7 +87,7 @@ enum { > static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int32_t)f; > } else { > return f | MAKE_64BIT_MASK(32, 32); > @@ -97,7 +97,7 @@ static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) > static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > { > /* Disable NaN-boxing check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint32_t)f; > } > > @@ -113,7 +113,7 @@ static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) > static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > { > /* the value is sign-extended instead of NaN-boxing for zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (int16_t)f; > } else { > return f | MAKE_64BIT_MASK(16, 48); > @@ -123,7 +123,7 @@ static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) > static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f) > { > /* Disable nanbox check when enable zfinx */ > - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { > + if (env_archcpu(env)->cfg.ext_zfinx) { > return (uint16_t)f; > } > > -- > 2.41.0 > >
On 2023/10/11 1:04, Richard Henderson wrote: > On 10/9/23 05:42, LIU Zhiwei wrote: >> >> On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >>> When CPUArchState* is available (here CPURISCVState*), we >>> can use the fast env_archcpu() macro to get ArchCPU* (here >>> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >>> when building with --enable-qom-cast-debug. >>> >>> Inspired-by: Richard W.M. Jones <rjones@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> >> >> By the way, does the community has the plan to support heterogeneous >> architecture cpus in one soc? > > Yes. Hi Richard, It's a good news. Thanks. > >> If so, maybe we have to do this qom cast somewhere. > > No, I don't think so. Or at least not in these places. Yes. Perhaps, we should remove all RISCV_CPU macros using after the qom objects realized. Do you think we should remove the RISCV_CPU using in riscv_cpu_exec_interrupt? Although it is not so hot. I think there is no reason to use it there. Except this, there are many other places in hw/ and target/riscv using the RISCV_CPU macro. If we know whether we should remove the RISCV_CPU macro use in these places, we can do it in another patch. Thanks, Zhiwei > > > r~
On 11/10/23 05:25, LIU Zhiwei wrote: > > On 2023/10/11 1:04, Richard Henderson wrote: >> On 10/9/23 05:42, LIU Zhiwei wrote: >>> >>> On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >>>> When CPUArchState* is available (here CPURISCVState*), we >>>> can use the fast env_archcpu() macro to get ArchCPU* (here >>>> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >>>> when building with --enable-qom-cast-debug. >>> If so, maybe we have to do this qom cast somewhere. >> >> No, I don't think so. Or at least not in these places. > > Yes. Perhaps, we should remove all RISCV_CPU macros using after the qom > objects realized. > > Do you think we should remove the RISCV_CPU using in > riscv_cpu_exec_interrupt? Although it is not so hot. I think there is > no reason to use it there. I have some note in my TODO to check replacing CPUState by ArchCPU in TCGCPUOps (like the cpu_exec_interrupt handler you mentioned). However I'm running out of time, so feel free to try it. Using ArchCPU avoids the cast in target code. > Except this, there are many other places in hw/ and target/riscv using > the RISCV_CPU macro. If a method is exposed as API, we need to check the type. After that for internal calls this is pointless. > If we know whether we should remove the RISCV_CPU macro use in these > places, we can do it in another patch. > > Thanks, > Zhiwei > >> >> >> r~
On 2023/10/11 13:31, Philippe Mathieu-Daudé wrote: > On 11/10/23 05:25, LIU Zhiwei wrote: >> >> On 2023/10/11 1:04, Richard Henderson wrote: >>> On 10/9/23 05:42, LIU Zhiwei wrote: >>>> >>>> On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >>>>> When CPUArchState* is available (here CPURISCVState*), we >>>>> can use the fast env_archcpu() macro to get ArchCPU* (here >>>>> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >>>>> when building with --enable-qom-cast-debug. > > >>>> If so, maybe we have to do this qom cast somewhere. >>> >>> No, I don't think so. Or at least not in these places. >> >> Yes. Perhaps, we should remove all RISCV_CPU macros using after the >> qom objects realized. >> >> Do you think we should remove the RISCV_CPU using in >> riscv_cpu_exec_interrupt? Although it is not so hot. I think there >> is no reason to use it there. > > I have some note in my TODO to check replacing CPUState by ArchCPU in > TCGCPUOps (like the cpu_exec_interrupt handler you mentioned). IMHO, this will make it harder for heterogeneous SOC support. ArchCPU is not a target agnostic struct. I must miss something. > However > I'm running out of time, so feel free to try it. I'd like to have a try if it will not block the heterogeneous SOC support. > > Using ArchCPU avoids the cast in target code. Yes > >> Except this, there are many other places in hw/ and target/riscv >> using the RISCV_CPU macro. > > If a method is exposed as API, we need to check the type. After that > for internal calls this is pointless. Make sense. Thanks. Zhiwei > >> If we know whether we should remove the RISCV_CPU macro use in these >> places, we can do it in another patch. >> >> Thanks, >> Zhiwei >> >>> >>> >>> r~
On 10/11/23 22:59, LIU Zhiwei wrote: > > On 2023/10/11 13:31, Philippe Mathieu-Daudé wrote: >> On 11/10/23 05:25, LIU Zhiwei wrote: >>> >>> On 2023/10/11 1:04, Richard Henderson wrote: >>>> On 10/9/23 05:42, LIU Zhiwei wrote: >>>>> >>>>> On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >>>>>> When CPUArchState* is available (here CPURISCVState*), we >>>>>> can use the fast env_archcpu() macro to get ArchCPU* (here >>>>>> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >>>>>> when building with --enable-qom-cast-debug. >> >> >>>>> If so, maybe we have to do this qom cast somewhere. >>>> >>>> No, I don't think so. Or at least not in these places. >>> >>> Yes. Perhaps, we should remove all RISCV_CPU macros using after the qom objects realized. >>> >>> Do you think we should remove the RISCV_CPU using in riscv_cpu_exec_interrupt? Although >>> it is not so hot. I think there is no reason to use it there. >> >> I have some note in my TODO to check replacing CPUState by ArchCPU in >> TCGCPUOps (like the cpu_exec_interrupt handler you mentioned). > > IMHO, this will make it harder for heterogeneous SOC support. ArchCPU is not a target > agnostic struct. ArchCPU is a target-agnostic typedef of a structure with no visible definition. C is perfectly happy to manipulate pointers to such structures. Whether it is worthwhile to adjust interfaces from CPUState to ArchCPU, I don't know. r~
On 2023/10/13 0:06, Richard Henderson wrote: > On 10/11/23 22:59, LIU Zhiwei wrote: >> >> On 2023/10/11 13:31, Philippe Mathieu-Daudé wrote: >>> On 11/10/23 05:25, LIU Zhiwei wrote: >>>> >>>> On 2023/10/11 1:04, Richard Henderson wrote: >>>>> On 10/9/23 05:42, LIU Zhiwei wrote: >>>>>> >>>>>> On 2023/10/9 19:02, Philippe Mathieu-Daudé wrote: >>>>>>> When CPUArchState* is available (here CPURISCVState*), we >>>>>>> can use the fast env_archcpu() macro to get ArchCPU* (here >>>>>>> RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower >>>>>>> when building with --enable-qom-cast-debug. >>> >>> >>>>>> If so, maybe we have to do this qom cast somewhere. >>>>> >>>>> No, I don't think so. Or at least not in these places. >>>> >>>> Yes. Perhaps, we should remove all RISCV_CPU macros using after >>>> the qom objects realized. >>>> >>>> Do you think we should remove the RISCV_CPU using in >>>> riscv_cpu_exec_interrupt? Although it is not so hot. I think there >>>> is no reason to use it there. >>> >>> I have some note in my TODO to check replacing CPUState by ArchCPU in >>> TCGCPUOps (like the cpu_exec_interrupt handler you mentioned). >> >> IMHO, this will make it harder for heterogeneous SOC support. ArchCPU >> is not a target agnostic struct. > > ArchCPU is a target-agnostic typedef of a structure with no visible > definition. > C is perfectly happy to manipulate pointers to such structures. > Get it. Thanks > Whether it is worthwhile to adjust interfaces from CPUState to > ArchCPU, I don't know. OK. Let's just wait for a good reason to do that. Zhiwei > > > r~
diff --git a/target/riscv/internals.h b/target/riscv/internals.h index b5f823c7ec..8239ae83cc 100644 --- a/target/riscv/internals.h +++ b/target/riscv/internals.h @@ -87,7 +87,7 @@ enum { static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) { /* the value is sign-extended instead of NaN-boxing for zfinx */ - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { + if (env_archcpu(env)->cfg.ext_zfinx) { return (int32_t)f; } else { return f | MAKE_64BIT_MASK(32, 32); @@ -97,7 +97,7 @@ static inline uint64_t nanbox_s(CPURISCVState *env, float32 f) static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) { /* Disable NaN-boxing check when enable zfinx */ - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { + if (env_archcpu(env)->cfg.ext_zfinx) { return (uint32_t)f; } @@ -113,7 +113,7 @@ static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f) static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) { /* the value is sign-extended instead of NaN-boxing for zfinx */ - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { + if (env_archcpu(env)->cfg.ext_zfinx) { return (int16_t)f; } else { return f | MAKE_64BIT_MASK(16, 48); @@ -123,7 +123,7 @@ static inline uint64_t nanbox_h(CPURISCVState *env, float16 f) static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f) { /* Disable nanbox check when enable zfinx */ - if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) { + if (env_archcpu(env)->cfg.ext_zfinx) { return (uint16_t)f; }
When CPUArchState* is available (here CPURISCVState*), we can use the fast env_archcpu() macro to get ArchCPU* (here RISCVCPU*). The QOM cast RISCV_CPU() macro will be slower when building with --enable-qom-cast-debug. Inspired-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- target/riscv/internals.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)