From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.17142.1684226914278602414 for ; Tue, 16 May 2023 01:48:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=koufoleV; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id C4270240038 for ; Tue, 16 May 2023 10:48:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1684226911; bh=3eCzik8DE9hIaxfOQmhQl19qgoZMPV+SMpRpwdH0Jwg=; h=From:Message-Id:Mime-Version:Subject:Date:Cc:To:From; b=koufoleVcDqrBV0exJgiyrSy4Y1ygy5g5UOgtgpvrd1/8/KY+hSyBd3uKl2UFXC8M O1v3N6tBhX+5w1t7e8b/p1VGwt1Ucj3DVGXuiokrCqEMQq8pfw8ZXwLZ+vJeyjdO9t cBG23x7+RZHWSxwRGt8E4OL0pW+TdjpBX3YV5+TBR6A5aftpopKpKwFJWGxJCSOTW5 ni1bYso6rtx4fKxVxuX/OK8M7bRnV7ZflJBZomTOz+hTQbWq/faZz9+0yvudDaHOKh LuO++kgOQBWvaFVLK22+S2zcH9KwroUOKwideZmz4h9UxY+MsbkVmfOaCC6tWy1cZ0 6yrCA17RXqtTA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4QL8z15SJrz6tsj; Tue, 16 May 2023 10:48:29 +0200 (CEST) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Subject: Re: [PATCH v3 1/1] MdePkg/Base.h: Simplify alignment expressions Date: Tue, 16 May 2023 08:48:19 +0000 In-Reply-To: <20230516021921.411852-1-pedro.falcato@gmail.com> Cc: edk2-devel-groups-io , Michael D Kinney , Liming Gao , Zhiguang Liu To: Pedro Falcato References: <20230516021921.411852-1-pedro.falcato@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_CAEAEB52-02A2-48BE-BB7C-495AB1A63077" --Apple-Mail=_CAEAEB52-02A2-48BE-BB7C-495AB1A63077 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 16. May 2023, at 04:19, Pedro Falcato = wrote: >=20 > =EF=BB=BFSimplify ALIGN_VALUE and ALIGN_VALUE_ADDEND into simpler = expressions. >=20 > 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) =3D (15 + 15) & ~16 =3D 16 > ALIGN_VALUE(16, 16) =3D (16 + 15) & ~16 =3D 16 >=20 > Old codegen: > movq %rdi, %rax > negq %rax > andl $15, %eax > addq %rdi, %rax >=20 > New codegen: > leaq 15(%rdi), %rax > andq $-16, %rax >=20 > ALIGN_VALUE_ADDEND can simply use the negation of Value to get the > addend for alignment, as, for instance: > -15 & (16 - 1) =3D 1 > 15 + 1 =3D 16 >=20 > 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. >=20 > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Cc: Marvin H=C3=A4user > Signed-off-by: Pedro Falcato > --- > 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(-) >=20 > 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) = =3D=3D sizeof (__VERIFY_INT32_ENUM >=20 > @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)) >=20 > /** > Rounds a value up to the next boundary using a specified alignment. > @@ -945,7 +945,7 @@ STATIC_ASSERT (ALIGNOF (__VERIFY_INT32_ENUM_SIZE) = =3D=3D sizeof (__VERIFY_INT32_ENUM > @return A value up to the next boundary. >=20 > **/ > -#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) >=3D = 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 >=20 > /** > Adjust a pointer by adding the minimum offset required for it to be = aligned on > --=20 > 2.40.1 >=20 --Apple-Mail=_CAEAEB52-02A2-48BE-BB7C-495AB1A63077 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 16. May 2023, at 04:19, Pedro = Falcato <pedro.falcato@gmail.com> = wrote:

=EF=BB=BFSimplify 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) =3D (15 + = 15) & ~16 =3D 16
   ALIGN_VALUE(16, 16) =3D= (16 + 15) & ~16 =3D 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) =3D = 1
   15 + 1 =3D = 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=C3=A4user = <mhaeuser@posteo.de>
Signed-off-by: Pedro Falcato = <pedro.falcato@gmail.com>
---
Change= s:
   - 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) =3D=3D 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) =3D=3D 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))

~((Alignmen= t) - 1U)

Also, I'm quite sure this breaks for = the case typeof(Value) > typeof(Alignment),typeof(Value) > = unsigned int,typeof(Alignment) >=3D 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

= --Apple-Mail=_CAEAEB52-02A2-48BE-BB7C-495AB1A63077--