| Message ID | 20250310045842.2650784-6-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | make system memory API available for common code | expand |
On 3/9/25 21:58, Pierrick Bouvier wrote: > For now, they are duplicate of the same macros in cpu-all.h that we > eliminate in next commit. > > Keep code readable by not defining them with macros, but simply their > implementation. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Why do you want these in bswap.h, rather than tswap.h? They're target swaps, after all. r~
On 3/10/25 09:08, Richard Henderson wrote: > On 3/9/25 21:58, Pierrick Bouvier wrote: >> For now, they are duplicate of the same macros in cpu-all.h that we >> eliminate in next commit. >> >> Keep code readable by not defining them with macros, but simply their >> implementation. >> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Why do you want these in bswap.h, rather than tswap.h? > They're target swaps, after all. > > > r~ No preference on that, I simply added them to the same file than their explicit endianness variant. Would you prefer the endianness agnostic variant to be in tswap.h instead?
On 3/10/25 09:14, Pierrick Bouvier wrote: > On 3/10/25 09:08, Richard Henderson wrote: >> On 3/9/25 21:58, Pierrick Bouvier wrote: >>> For now, they are duplicate of the same macros in cpu-all.h that we >>> eliminate in next commit. >>> >>> Keep code readable by not defining them with macros, but simply their >>> implementation. >>> >>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> >> Why do you want these in bswap.h, rather than tswap.h? >> They're target swaps, after all. >> >> >> r~ > > No preference on that, I simply added them to the same file than their explicit endianness > variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? I think I would. In addition, I think we want #ifdef COMPILING_PER_TARGET #define target_words_bigendian() TARGET_BIG_ENDIAN #else bool target_words_bigendian(void); #endif moving the conditional from around target_needs_bswap just below. With that, we eliminate the extra branch that you're otherwise adding to target-specific code with this patch. r~
On 3/10/25 09:37, Richard Henderson wrote: > On 3/10/25 09:14, Pierrick Bouvier wrote: >> On 3/10/25 09:08, Richard Henderson wrote: >>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>> eliminate in next commit. >>>> >>>> Keep code readable by not defining them with macros, but simply their >>>> implementation. >>>> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> >>> Why do you want these in bswap.h, rather than tswap.h? >>> They're target swaps, after all. >>> >>> >>> r~ >> >> No preference on that, I simply added them to the same file than their explicit endianness >> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? > > I think I would. Ok, I will move it. > > In addition, I think we want > > #ifdef COMPILING_PER_TARGET > #define target_words_bigendian() TARGET_BIG_ENDIAN > #else > bool target_words_bigendian(void); > #endif > > moving the conditional from around target_needs_bswap just below. > > With that, we eliminate the extra branch that you're otherwise > adding to target-specific code with this patch. > I understand the change requested, but should we really aim in that direction? In the end, if we pursue the compilation units deduplication, the branch will be present anyway. I'm ok with your change, just asking if we really want to preserve target specific code until the "end". > > r~
On 3/10/25 09:43, Pierrick Bouvier wrote: > On 3/10/25 09:37, Richard Henderson wrote: >> On 3/10/25 09:14, Pierrick Bouvier wrote: >>> On 3/10/25 09:08, Richard Henderson wrote: >>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>> eliminate in next commit. >>>>> >>>>> Keep code readable by not defining them with macros, but simply their >>>>> implementation. >>>>> >>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> >>>> Why do you want these in bswap.h, rather than tswap.h? >>>> They're target swaps, after all. >>>> >>>> >>>> r~ >>> >>> No preference on that, I simply added them to the same file than their explicit endianness >>> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? >> >> I think I would. > > Ok, I will move it. > >> >> In addition, I think we want >> >> #ifdef COMPILING_PER_TARGET >> #define target_words_bigendian() TARGET_BIG_ENDIAN >> #else >> bool target_words_bigendian(void); >> #endif >> >> moving the conditional from around target_needs_bswap just below. >> >> With that, we eliminate the extra branch that you're otherwise >> adding to target-specific code with this patch. >> > > I understand the change requested, but should we really aim in that direction? In the end, > if we pursue the compilation units deduplication, the branch will be present anyway. > > I'm ok with your change, just asking if we really want to preserve target specific code > until the "end". All of target/ is target specific. De-duplication will not eliminate that. r~
On 3/10/25 09:53, Richard Henderson wrote: > On 3/10/25 09:43, Pierrick Bouvier wrote: >> On 3/10/25 09:37, Richard Henderson wrote: >>> On 3/10/25 09:14, Pierrick Bouvier wrote: >>>> On 3/10/25 09:08, Richard Henderson wrote: >>>>> On 3/9/25 21:58, Pierrick Bouvier wrote: >>>>>> For now, they are duplicate of the same macros in cpu-all.h that we >>>>>> eliminate in next commit. >>>>>> >>>>>> Keep code readable by not defining them with macros, but simply their >>>>>> implementation. >>>>>> >>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>>> >>>>> Why do you want these in bswap.h, rather than tswap.h? >>>>> They're target swaps, after all. >>>>> >>>>> >>>>> r~ >>>> >>>> No preference on that, I simply added them to the same file than their explicit endianness >>>> variant. Would you prefer the endianness agnostic variant to be in tswap.h instead? >>> >>> I think I would. >> >> Ok, I will move it. >> >>> >>> In addition, I think we want >>> >>> #ifdef COMPILING_PER_TARGET >>> #define target_words_bigendian() TARGET_BIG_ENDIAN >>> #else >>> bool target_words_bigendian(void); >>> #endif >>> >>> moving the conditional from around target_needs_bswap just below. >>> >>> With that, we eliminate the extra branch that you're otherwise >>> adding to target-specific code with this patch. >>> >> >> I understand the change requested, but should we really aim in that direction? In the end, >> if we pursue the compilation units deduplication, the branch will be present anyway. >> >> I'm ok with your change, just asking if we really want to preserve target specific code >> until the "end". > > All of target/ is target specific. De-duplication will not eliminate that. > My vocabulary was wrong here. I meant "if we want to preserve target specific macros" until the end. Sure, there will always be compilation units (devices, cpus, helpers, ...) specific to a target. I just wonder if sticking to ifdef paradigm for this kind of code is worth the "optimization" we are supposed to get. I'll add the change requested. > > r~
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h index ebf6f9e5f5c..46ec62f716d 100644 --- a/include/qemu/bswap.h +++ b/include/qemu/bswap.h @@ -442,6 +442,76 @@ DO_STN_LDN_P(be) #undef DO_STN_LDN_P +/* Return ld{word}_{le,be}_p following target endianness. */ +#define LOAD_IMPL(word, args...) \ +do { \ + if (target_words_bigendian()) { \ + return glue(glue(ld, word), _be_p)(args); \ + } else { \ + return glue(glue(ld, word), _le_p)(args); \ + } \ +} while (0) + +static inline int lduw_p(const void *ptr) +{ + LOAD_IMPL(uw, ptr); +} + +static inline int ldsw_p(const void *ptr) +{ + LOAD_IMPL(sw, ptr); +} + +static inline int ldl_p(const void *ptr) +{ + LOAD_IMPL(l, ptr); +} + +static inline uint64_t ldq_p(const void *ptr) +{ + LOAD_IMPL(q, ptr); +} + +static inline uint64_t ldn_p(const void *ptr, int sz) +{ + LOAD_IMPL(n, ptr, sz); +} + +#undef LOAD_IMPL + +/* Call st{word}_{le,be}_p following target endianness. */ +#define STORE_IMPL(word, args...) \ +do { \ + if (target_words_bigendian()) { \ + glue(glue(st, word), _be_p)(args); \ + } else { \ + glue(glue(st, word), _le_p)(args); \ + } \ +} while (0) + + +static inline void stw_p(void *ptr, uint16_t v) +{ + STORE_IMPL(w, ptr, v); +} + +static inline void stl_p(void *ptr, uint32_t v) +{ + STORE_IMPL(l, ptr, v); +} + +static inline void stq_p(void *ptr, uint64_t v) +{ + STORE_IMPL(q, ptr, v); +} + +static inline void stn_p(void *ptr, int sz, uint64_t v) +{ + STORE_IMPL(n, ptr, sz, v); +} + +#undef STORE_IMPL + #undef le_bswap #undef be_bswap #undef le_bswaps
For now, they are duplicate of the same macros in cpu-all.h that we eliminate in next commit. Keep code readable by not defining them with macros, but simply their implementation. Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- include/qemu/bswap.h | 70 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)