回复: [edk2-devel] [PATCH v2 1/1] MdePkg/Base.h: Simply alignment expressions

["gaoliming" <gaoliming@byosoft.com.cn>]

Pedro:

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Pedro Falcato
> 发送时间: 2023年5月15日 23:15
> 收件人: devel@edk2.groups.io
> 抄送: Pedro Falcato <pedro.falcato@gmail.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Marvin Häuser
> <mhaeuser@posteo.de>
> 主题: [edk2-devel] [PATCH v2 1/1] MdePkg/Base.h: Simply alignment
> expressions
> 
> Simplify ALIGN_VALUE and ALIGN_VALUE_ADDEND into simpler expressions.
> 
> ALIGN_VALUE can simply be a (value + (align - 1)) & ~align
> expression, which works for any power of 2 alignment and generates
> smaller code sequences. For instance:
> 	ALIGN_VALUE(15, 16) = (15 + 15) & ~16 = 16
> 	ALIGN_VALUE(16, 16) = (16 + 15) & ~16 = 16
> 
> Old codegen:
> 	movq    %rdi, %rax
> 	negq    %rax
> 	andl    $15, %eax
> 	addq    %rdi, %rax
> 
> New codegen:
> 	leaq    15(%rdi), %rax
> 	andq    $-16, %rax
> 
> ALIGN_VALUE_ADDEND can simply use a bitwise NOT of Value to get the
> addend for alignment, as, for instance:
> 	~15 & (16 - 1) = 1
> 	15 + 1 = 16
> 

> 	~15 & (16 - 1) = 1
Its value should be zero, not 1. I also verify the updated ALIGN_VALUE_ADDEND. 
Its value is incorrect. Please double check. 

Thanks
Liming

> This change does not necessarily affect the end result, as the GCC and
> clang compilers were already able to see through things and optimize
> them into optimal instruction sequences, in the ALIGN_VALUE_ADDEND case.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
> 
> v2:
> 	Addressed concerns expressed on Discord by Marvin
> 		- Added missing parens around Alignment in ALIGN_VALUE
> 		- Replaced -1 with -1U, as in the other macros.
> 
>  MdePkg/Include/Base.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> index 6597e441a6e2..422f80aff53d 100644
> --- a/MdePkg/Include/Base.h
> +++ b/MdePkg/Include/Base.h
> @@ -931,7 +931,7 @@ STATIC_ASSERT (ALIGNOF
> (__VERIFY_INT32_ENUM_SIZE) == sizeof (__VERIFY_INT32_ENUM
> 
>    @return  Addend to round Value up to alignment boundary Alignment.
>  **/
> -#define ALIGN_VALUE_ADDEND(Value, Alignment)  (((Alignment) - (Value))
> & ((Alignment) - 1U))
> +#define ALIGN_VALUE_ADDEND(Value, Alignment)  ((~(Value)) &
> ((Alignment) - 1U))
> 
>  /**
>    Rounds a value up to the next boundary using a specified alignment.
> @@ -945,7 +945,7 @@ STATIC_ASSERT (ALIGNOF
> (__VERIFY_INT32_ENUM_SIZE) == sizeof (__VERIFY_INT32_ENUM
>    @return  A value up to the next boundary.
> 
>  **/
> -#define ALIGN_VALUE(Value, Alignment)  ((Value) +
> ALIGN_VALUE_ADDEND (Value, Alignment))
> +#define ALIGN_VALUE(Value, Alignment)  (((Value) + ((Alignment) - 1U)) &
> ~(Alignment))
> 
>  /**
>    Adjust a pointer by adding the minimum offset required for it to be aligned
> on
> --
> 2.40.1
> 
> 
> 
> 
>