Hi,
On 10/05/2022 11:15, Lin Liu wrote:
> swab() is massively over complicated and can be simplified by builtins.
NIT: "by builtins" -> "by re-implementing using compiler builtins".
> The compilers provide builtin function to swap bytes.
> * gcc:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FOther-Builtins.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HDTF1LDJcD2PLSCuM%2FjIz%2FWGf1CrYk0e%2FLox22%2FXnvQ%3D&reserved=0
> * clang:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fclang.llvm.org%2Fdocs%2FLanguageExtensions.html&data=05%7C01%7Clin.liu%40citrix.com%7Ce0b3d98d7f8d47b8fe8708da3275afcd%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637877778294067911%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EvWcLMi2vtT9haQVo%2F9uXmjBh2zVLUzZAgU57i%2FFMNo%3D&reserved=0
> This patch simplify swab() with builtins and fallback for old compilers.
>
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> Cc: Bertrand Marquis <bertrand.marquis@arm.com>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monn¨¦" <roger.pau@citrix.com>
> Changes in v3:
> - Check __has_builtin instead of GNUC version
>
> Changes in v2:
> - Add fallback for compilers without __builtin_bswap
> - Implement with plain C instead of macros
> ---
> xen/arch/arm/include/asm/byteorder.h | 14 ++-----
> xen/arch/x86/include/asm/byteorder.h | 34 ++---------------
> xen/include/xen/byteorder.h | 56 ++++++++++++++++++++++++++++
> xen/include/xen/byteswap.h | 44 ++++++++++++++++++++++
> xen/include/xen/compiler.h | 12 ++++++
> 5 files changed, 120 insertions(+), 40 deletions(-)
> create mode 100644 xen/include/xen/byteorder.h
> create mode 100644 xen/include/xen/byteswap.h
>
> diff --git a/xen/arch/arm/include/asm/byteorder.h b/xen/arch/arm/include/asm/byteorder.h
> index 9c712c4788..622eeaba07 100644
> --- a/xen/arch/arm/include/asm/byteorder.h
> +++ b/xen/arch/arm/include/asm/byteorder.h
> @@ -1,16 +1,10 @@
> #ifndef __ASM_ARM_BYTEORDER_H__
> #define __ASM_ARM_BYTEORDER_H__
>
> -#define __BYTEORDER_HAS_U64__
> +#ifndef __BYTE_ORDER__
> + #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
> +#endif
>
> -#include <xen/byteorder/little_endian.h>
> +#include <xen/byteorder.h>
>
> #endif /* __ASM_ARM_BYTEORDER_H__ */
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
>> This change looks unrelated to this patch. Can you explain it?
Do you mean following code block? Yes, it is unrelated, I removed it as I found some files does not include such field.
Will revert such field in V4.
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * indent-tabs-mode: nil
> - * End:
> - */