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

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

[Pedro Falcato]

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))
 
 /**
   Adjust a pointer by adding the minimum offset required for it to be aligned on
-- 
2.40.1

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

[Marvin Häuser]

[-- 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 --]