Re: [PATCH v3 1/1] MdePkg/Base.h: Simplify alignment expressions

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3153 bytes --]


> On 16. May 2023, at 04:19, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> 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 the negation of Value to get the
> addend for alignment, as, for instance:
>    -15 & (16 - 1) = 1
>    15 + 1 = 16
> 
> 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>
> ---
> Changes:
>    - Correct the ADDEND macro to use negation and not a binary NOT (thanks Liming!)
> 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..a070593a360d 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))

~((Alignment) - 1U)

Also, I'm quite sure this breaks for the case typeof(Value) > typeof(Alignment),typeof(Value) > unsigned int,typeof(Alignment) >= unsigned int (but always unsigned), this breaks. Assume Value is unsigned long long and Alignment is unsigned int. Then, the left-hand side will be unsigned long long. ~((Alignment) - 1U) will be unsigned int (regardless of whether or not we use 1 or 1U). Now, it will be promoted to unsigned long long for the evaluation of the AND operation, but because it is unsigned, it will not "sign-extend", thus discarding the high Bits of Value.

Best regards,
Marvin

> 
> /**
>   Adjust a pointer by adding the minimum offset required for it to be aligned on
> -- 
> 2.40.1
> 

[-- Attachment #2: Type: text/html, Size: 4918 bytes --]