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

[PATCH 1/1] MdePkg/Base.h: Simply 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 a bitwise NOT 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>
---
 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..eba1178e3497 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 - 1)) & ~(Alignment))
 
 /**
   Adjust a pointer by adding the minimum offset required for it to be aligned on
-- 
2.40.1

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

[Rebecca Cran]

On 5/15/23 08:45, Pedro Falcato wrote:
> -#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 - 1)) & ~(Alignment))

Since ALIGN_VALUE_ADDEND is only used in ALIGN_VALUE, it should probably 
be deleted instead of updated.


-- 

Rebecca Cran

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

[Marvin Häuser]

Well, I explicitly added this macro as a prerequisite to code used in our new PE library (remember this patch was initially sent in 2021). We still require it downstream, but obviously upstream is not interested in the related contributions that were to follow at the time.

Gerd picked it up because he wanted to attempt to re-try contributing the new PE library, but I haven't heard from him in weeks.

Design-wise, I agree it could be removed again. However, there first was a downstream burden when adding it (as we needed to rewrite our history to drop our downstream patch in favour of the upstream solution). Now introducing another downstream burden *again* to remove the macro that was added only a few weeks back would be a sign of poor management and planning.

Best regards,
Marvin

> On 15. May 2023, at 17:15, Rebecca Cran <rebecca@bsdio.com> wrote:
> 
> On 5/15/23 08:45, Pedro Falcato wrote:
>> -#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 - 1)) & ~(Alignment))
> 
> Since ALIGN_VALUE_ADDEND is only used in ALIGN_VALUE, it should probably be deleted instead of updated.
> 
> 
> -- 
> 
> Rebecca Cran
> 

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

[Pedro Falcato]

On Mon, May 15, 2023 at 4:20 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Well, I explicitly added this macro as a prerequisite to code used in our new PE library (remember this patch was initially sent in 2021). We still require it downstream, but obviously upstream is not interested in the related contributions that were to follow at the time.
>
> Gerd picked it up because he wanted to attempt to re-try contributing the new PE library, but I haven't heard from him in weeks.
>
> Design-wise, I agree it could be removed again. However, there first was a downstream burden when adding it (as we needed to rewrite our history to drop our downstream patch in favour of the upstream solution). Now introducing another downstream burden *again* to remove the macro that was added only a few weeks back would be a sign of poor management and planning.

I don't agree with the removal of the macro. It's useful enough to
consumers of Base.h, and clearly there's code that is indeed actively
using it.

It's also a single line of code.

-- 
Pedro

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

[Rebecca Cran]

I wasn’t aware of the history, so I agree with keeping it.

— 
Rebecca Cran

On Mon, May 15, 2023, at 9:29 AM, Pedro Falcato wrote:
> On Mon, May 15, 2023 at 4:20 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>> Well, I explicitly added this macro as a prerequisite to code used in our new PE library (remember this patch was initially sent in 2021). We still require it downstream, but obviously upstream is not interested in the related contributions that were to follow at the time.
>>
>> Gerd picked it up because he wanted to attempt to re-try contributing the new PE library, but I haven't heard from him in weeks.
>>
>> Design-wise, I agree it could be removed again. However, there first was a downstream burden when adding it (as we needed to rewrite our history to drop our downstream patch in favour of the upstream solution). Now introducing another downstream burden *again* to remove the macro that was added only a few weeks back would be a sign of poor management and planning.
>
> I don't agree with the removal of the macro. It's useful enough to
> consumers of Base.h, and clearly there's code that is indeed actively
> using it.
>
> It's also a single line of code.
>
> -- 
> Pedro
>
>
>