* [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
@ 2017-12-07 15:12 Ard Biesheuvel
2017-12-07 15:26 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 15:12 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, liming.gao, evan.lloyd, michael.d.kinney,
Alexei.Fedorov, Ard Biesheuvel
ASSERT_EFI_ERROR () is currently defined as
#if !defined(MDEPKG_NDEBUG)
#define ASSERT_EFI_ERROR(StatusParameter) \
do { \
if (DebugAssertEnabled ()) { \
if (EFI_ERROR (StatusParameter)) { \
DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n", StatusParameter)); \
_ASSERT (!EFI_ERROR (StatusParameter)); \
} \
} \
} while (FALSE)
#else
#define ASSERT_EFI_ERROR(StatusParameter)
#endif
This is suboptimal, given that the DebugAssertEnabled () call in the
outer if must be executed unconditionally, since the compiler does not
know that it does not have any side effects. Instead, let's swap the
two ifs, and only call DebugAssertEnabled () if StatusParameter contains
an error value to begin with. Do the same for ASSERT_RETURN_ERROR
as well.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
---
MdePkg/Include/Library/DebugLib.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index 3a910e6a208b..8369c378e79c 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -337,8 +337,8 @@ DebugPrintLevelEnabled (
#if !defined(MDEPKG_NDEBUG)
#define ASSERT_EFI_ERROR(StatusParameter) \
do { \
- if (DebugAssertEnabled ()) { \
- if (EFI_ERROR (StatusParameter)) { \
+ if (EFI_ERROR (StatusParameter)) { \
+ if (DebugAssertEnabled ()) { \
DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n", StatusParameter)); \
_ASSERT (!EFI_ERROR (StatusParameter)); \
} \
@@ -363,8 +363,8 @@ DebugPrintLevelEnabled (
#if !defined(MDEPKG_NDEBUG)
#define ASSERT_RETURN_ERROR(StatusParameter) \
do { \
- if (DebugAssertEnabled ()) { \
- if (RETURN_ERROR (StatusParameter)) { \
+ if (RETURN_ERROR (StatusParameter)) { \
+ if (DebugAssertEnabled ()) { \
DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
StatusParameter)); \
_ASSERT (!RETURN_ERROR (StatusParameter)); \
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 15:12 [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR Ard Biesheuvel
@ 2017-12-07 15:26 ` Ard Biesheuvel
2017-12-07 17:01 ` Kinney, Michael D
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 15:26 UTC (permalink / raw)
To: edk2-devel@lists.01.org
Cc: Leif Lindholm, Gao, Liming, Evan Lloyd, Kinney, Michael D,
Alexei Fedorov, Ard Biesheuvel
On 7 December 2017 at 15:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> ASSERT_EFI_ERROR () is currently defined as
>
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT_EFI_ERROR(StatusParameter) \
> do { \
> if (DebugAssertEnabled ()) { \
> if (EFI_ERROR (StatusParameter)) { \
> DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n", StatusParameter)); \
> _ASSERT (!EFI_ERROR (StatusParameter)); \
> } \
> } \
> } while (FALSE)
> #else
> #define ASSERT_EFI_ERROR(StatusParameter)
> #endif
>
> This is suboptimal, given that the DebugAssertEnabled () call in the
> outer if must be executed unconditionally, since the compiler does not
> know that it does not have any side effects. Instead, let's swap the
> two ifs, and only call DebugAssertEnabled () if StatusParameter contains
> an error value to begin with. Do the same for ASSERT_RETURN_ERROR
> as well.
>
I just noticed we could do the same for ASSERT () as well.
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
> ---
> MdePkg/Include/Library/DebugLib.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> index 3a910e6a208b..8369c378e79c 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -337,8 +337,8 @@ DebugPrintLevelEnabled (
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT_EFI_ERROR(StatusParameter) \
> do { \
> - if (DebugAssertEnabled ()) { \
> - if (EFI_ERROR (StatusParameter)) { \
> + if (EFI_ERROR (StatusParameter)) { \
> + if (DebugAssertEnabled ()) { \
> DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n", StatusParameter)); \
> _ASSERT (!EFI_ERROR (StatusParameter)); \
> } \
> @@ -363,8 +363,8 @@ DebugPrintLevelEnabled (
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT_RETURN_ERROR(StatusParameter) \
> do { \
> - if (DebugAssertEnabled ()) { \
> - if (RETURN_ERROR (StatusParameter)) { \
> + if (RETURN_ERROR (StatusParameter)) { \
> + if (DebugAssertEnabled ()) { \
> DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> StatusParameter)); \
> _ASSERT (!RETURN_ERROR (StatusParameter)); \
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 15:26 ` Ard Biesheuvel
@ 2017-12-07 17:01 ` Kinney, Michael D
2017-12-07 17:09 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Kinney, Michael D @ 2017-12-07 17:01 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Leif Lindholm, Gao, Liming, Evan Lloyd, Alexei Fedorov
Ard,
The reason for the current ordering is for size optimization.
The most common implementation of DebugAssertEnabled() uses
a FixedAtBuild PCD to determine if these are enabled. The
check of status can be optimized away if they are disabled.
If you reverse them, then the status check is always performed.
Mike
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, December 7, 2017 7:26 AM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Evan Lloyd <evan.lloyd@arm.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Alexei
> Fedorov <Alexei.Fedorov@arm.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH] MdePkg/DebugLib; swap if conditions
> in ASSERT_[EFI|RETURN]_ERROR
>
> On 7 December 2017 at 15:12, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > ASSERT_EFI_ERROR () is currently defined as
> >
> > #if !defined(MDEPKG_NDEBUG)
> > #define ASSERT_EFI_ERROR(StatusParameter)
> \
> > do {
> \
> > if (DebugAssertEnabled ()) {
> \
> > if (EFI_ERROR (StatusParameter)) {
> \
> > DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR
> (Status = %r)\n", StatusParameter)); \
> > _ASSERT (!EFI_ERROR (StatusParameter));
> \
> > }
> \
> > }
> \
> > } while (FALSE)
> > #else
> > #define ASSERT_EFI_ERROR(StatusParameter)
> > #endif
> >
> > This is suboptimal, given that the DebugAssertEnabled
> () call in the
> > outer if must be executed unconditionally, since the
> compiler does not
> > know that it does not have any side effects. Instead,
> let's swap the
> > two ifs, and only call DebugAssertEnabled () if
> StatusParameter contains
> > an error value to begin with. Do the same for
> ASSERT_RETURN_ERROR
> > as well.
> >
>
> I just noticed we could do the same for ASSERT () as
> well.
>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> > Reported-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
> > ---
> > MdePkg/Include/Library/DebugLib.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h
> b/MdePkg/Include/Library/DebugLib.h
> > index 3a910e6a208b..8369c378e79c 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -337,8 +337,8 @@ DebugPrintLevelEnabled (
> > #if !defined(MDEPKG_NDEBUG)
> > #define ASSERT_EFI_ERROR(StatusParameter)
> \
> > do {
> \
> > - if (DebugAssertEnabled ()) {
> \
> > - if (EFI_ERROR (StatusParameter)) {
> \
> > + if (EFI_ERROR (StatusParameter)) {
> \
> > + if (DebugAssertEnabled ()) {
> \
> > DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR
> (Status = %r)\n", StatusParameter)); \
> > _ASSERT (!EFI_ERROR (StatusParameter));
> \
> > }
> \
> > @@ -363,8 +363,8 @@ DebugPrintLevelEnabled (
> > #if !defined(MDEPKG_NDEBUG)
> > #define ASSERT_RETURN_ERROR(StatusParameter)
> \
> > do {
> \
> > - if (DebugAssertEnabled ()) {
> \
> > - if (RETURN_ERROR (StatusParameter)) {
> \
> > + if (RETURN_ERROR (StatusParameter)) {
> \
> > + if (DebugAssertEnabled ()) {
> \
> > DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR
> (Status = %r)\n", \
> > StatusParameter));
> \
> > _ASSERT (!RETURN_ERROR (StatusParameter));
> \
> > --
> > 2.11.0
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 17:01 ` Kinney, Michael D
@ 2017-12-07 17:09 ` Ard Biesheuvel
2017-12-07 17:13 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 17:09 UTC (permalink / raw)
To: Kinney, Michael D
Cc: edk2-devel@lists.01.org, Leif Lindholm, Gao, Liming, Evan Lloyd,
Alexei Fedorov
On 7 December 2017 at 17:01, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Ard,
>
> The reason for the current ordering is for size optimization.
>
> The most common implementation of DebugAssertEnabled() uses
> a FixedAtBuild PCD to determine if these are enabled. The
> check of status can be optimized away if they are disabled.
> If you reverse them, then the status check is always performed.
>
DebugAssertEnabled() is a function call that gets resolved at link
time, and is not annotated as being free of side effects. So I agree
that the implementation of that function could be optimized into a
'return true' or 'return false' depending on the compile time values
of those PCDs, but the way the macro is defined currently, it still
requires the function call to be made, and the conditional compare
with a constant that follows will still be present in the code.
What I am suggesting is to replace it with a comparison with a
constant, and a conditional function call instead. This will not
affect code size, but will only remove needless function calls at
runtime.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 17:09 ` Ard Biesheuvel
@ 2017-12-07 17:13 ` Ard Biesheuvel
2017-12-07 17:36 ` Kinney, Michael D
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 17:13 UTC (permalink / raw)
To: Kinney, Michael D
Cc: edk2-devel@lists.01.org, Leif Lindholm, Gao, Liming, Evan Lloyd,
Alexei Fedorov
On 7 December 2017 at 17:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 7 December 2017 at 17:01, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
>> Ard,
>>
>> The reason for the current ordering is for size optimization.
>>
>> The most common implementation of DebugAssertEnabled() uses
>> a FixedAtBuild PCD to determine if these are enabled. The
>> check of status can be optimized away if they are disabled.
>> If you reverse them, then the status check is always performed.
>>
>
> DebugAssertEnabled() is a function call that gets resolved at link
> time, and is not annotated as being free of side effects. So I agree
> that the implementation of that function could be optimized into a
> 'return true' or 'return false' depending on the compile time values
> of those PCDs, but the way the macro is defined currently, it still
> requires the function call to be made, and the conditional compare
> with a constant that follows will still be present in the code.
>
> What I am suggesting is to replace it with a comparison with a
> constant, and a conditional function call instead. This will not
> affect code size, but will only remove needless function calls at
> runtime.
Please refer to these threads for details:
https://lists.01.org/pipermail/edk2-devel/2017-December/018790.html
https://lists.01.org/pipermail/edk2-devel/2017-December/018809.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 17:13 ` Ard Biesheuvel
@ 2017-12-07 17:36 ` Kinney, Michael D
2017-12-07 17:43 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Kinney, Michael D @ 2017-12-07 17:36 UTC (permalink / raw)
To: Ard Biesheuvel, Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Gao, Liming,
Leif Lindholm
Ard,
With link time optimization, the current order produces
smaller code.
Without link time optimization, your patch will produce
smaller code, but not as small as link time optimized code.
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Ard Biesheuvel
> Sent: Thursday, December 7, 2017 9:13 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
> devel@lists.01.org; Gao, Liming <liming.gao@intel.com>;
> Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
> conditions in ASSERT_[EFI|RETURN]_ERROR
>
> On 7 December 2017 at 17:09, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On 7 December 2017 at 17:01, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> >> Ard,
> >>
> >> The reason for the current ordering is for size
> optimization.
> >>
> >> The most common implementation of DebugAssertEnabled()
> uses
> >> a FixedAtBuild PCD to determine if these are enabled.
> The
> >> check of status can be optimized away if they are
> disabled.
> >> If you reverse them, then the status check is always
> performed.
> >>
> >
> > DebugAssertEnabled() is a function call that gets
> resolved at link
> > time, and is not annotated as being free of side
> effects. So I agree
> > that the implementation of that function could be
> optimized into a
> > 'return true' or 'return false' depending on the
> compile time values
> > of those PCDs, but the way the macro is defined
> currently, it still
> > requires the function call to be made, and the
> conditional compare
> > with a constant that follows will still be present in
> the code.
> >
> > What I am suggesting is to replace it with a comparison
> with a
> > constant, and a conditional function call instead. This
> will not
> > affect code size, but will only remove needless
> function calls at
> > runtime.
>
> Please refer to these threads for details:
> https://lists.01.org/pipermail/edk2-devel/2017-
> December/018790.html
> https://lists.01.org/pipermail/edk2-devel/2017-
> December/018809.html
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 17:36 ` Kinney, Michael D
@ 2017-12-07 17:43 ` Ard Biesheuvel
2017-12-07 19:49 ` Kinney, Michael D
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 17:43 UTC (permalink / raw)
To: Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Gao, Liming,
Leif Lindholm
On 7 December 2017 at 17:36, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Ard,
>
> With link time optimization, the current order produces
> smaller code.
>
I don't think it does. You are essentially saying that
DebugAssertEnabled() may resolve to a link time constant FALSE under
LTO.
In that case, why would the following two statement not be equivalent?
if (FALSE && EFI_ERROR (StatusParameter)) {}
if (EFI_ERROR (StatusParameter) && FALSE) {}
(which is essentially what a nested if () resolves to)
In other words, the compiler is smart enough to drop the status check
in the second case, because it can see there are no side effects, and
the condition can never be made true anyway.
> Without link time optimization, your patch will produce
> smaller code, but not as small as link time optimized code.
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 17:43 ` Ard Biesheuvel
@ 2017-12-07 19:49 ` Kinney, Michael D
2017-12-07 19:52 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Kinney, Michael D @ 2017-12-07 19:49 UTC (permalink / raw)
To: Ard Biesheuvel, Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Gao, Liming,
Leif Lindholm
Ard,
I do not disagree with your logic.
The current algorithm is based on data from a long
time ago using what are now very old tool chains.
I will do some experiments on the currently supported
toolchains to see if the optimization is the same either
way.
I think the change you are suggesting is to improve
performance for optimization disabled builds by removing
an extra call. Is that correct?
Mike
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, December 7, 2017 9:43 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
> devel@lists.01.org; Gao, Liming <liming.gao@intel.com>;
> Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
> conditions in ASSERT_[EFI|RETURN]_ERROR
>
> On 7 December 2017 at 17:36, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> > Ard,
> >
> > With link time optimization, the current order produces
> > smaller code.
> >
>
> I don't think it does. You are essentially saying that
> DebugAssertEnabled() may resolve to a link time constant
> FALSE under
> LTO.
>
> In that case, why would the following two statement not
> be equivalent?
>
> if (FALSE && EFI_ERROR (StatusParameter)) {}
>
> if (EFI_ERROR (StatusParameter) && FALSE) {}
>
> (which is essentially what a nested if () resolves to)
>
> In other words, the compiler is smart enough to drop the
> status check
> in the second case, because it can see there are no side
> effects, and
> the condition can never be made true anyway.
>
> > Without link time optimization, your patch will produce
> > smaller code, but not as small as link time optimized
> code.
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 19:49 ` Kinney, Michael D
@ 2017-12-07 19:52 ` Ard Biesheuvel
2017-12-07 20:33 ` Kinney, Michael D
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 19:52 UTC (permalink / raw)
To: Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Gao, Liming,
Leif Lindholm
On 7 December 2017 at 19:49, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Ard,
>
> I do not disagree with your logic.
>
> The current algorithm is based on data from a long
> time ago using what are now very old tool chains.
>
With LTO?
> I will do some experiments on the currently supported
> toolchains to see if the optimization is the same either
> way.
>
Thank you.
> I think the change you are suggesting is to improve
> performance for optimization disabled builds by removing
> an extra call. Is that correct?
>
Well, for DEBUG builds, yes. But given that the function call cannot
be optimized away (on non-LTO builds), it affects optimized builds as
well.
--
Ard.
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Thursday, December 7, 2017 9:43 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
>> devel@lists.01.org; Gao, Liming <liming.gao@intel.com>;
>> Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
>> conditions in ASSERT_[EFI|RETURN]_ERROR
>>
>> On 7 December 2017 at 17:36, Kinney, Michael D
>> <michael.d.kinney@intel.com> wrote:
>> > Ard,
>> >
>> > With link time optimization, the current order produces
>> > smaller code.
>> >
>>
>> I don't think it does. You are essentially saying that
>> DebugAssertEnabled() may resolve to a link time constant
>> FALSE under
>> LTO.
>>
>> In that case, why would the following two statement not
>> be equivalent?
>>
>> if (FALSE && EFI_ERROR (StatusParameter)) {}
>>
>> if (EFI_ERROR (StatusParameter) && FALSE) {}
>>
>> (which is essentially what a nested if () resolves to)
>>
>> In other words, the compiler is smart enough to drop the
>> status check
>> in the second case, because it can see there are no side
>> effects, and
>> the condition can never be made true anyway.
>>
>> > Without link time optimization, your patch will produce
>> > smaller code, but not as small as link time optimized
>> code.
>> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 19:52 ` Ard Biesheuvel
@ 2017-12-07 20:33 ` Kinney, Michael D
2017-12-07 20:42 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Kinney, Michael D @ 2017-12-07 20:33 UTC (permalink / raw)
To: Ard Biesheuvel, Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Leif Lindholm,
Gao, Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Ard Biesheuvel
> Sent: Thursday, December 7, 2017 11:53 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
> devel@lists.01.org; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
> conditions in ASSERT_[EFI|RETURN]_ERROR
>
> On 7 December 2017 at 19:49, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> > Ard,
> >
> > I do not disagree with your logic.
> >
> > The current algorithm is based on data from a long
> > time ago using what are now very old tool chains.
> >
>
> With LTO?
Yes. The LTCG feature for VS tool chains.
>
> > I will do some experiments on the currently supported
> > toolchains to see if the optimization is the same
> either
> > way.
> >
>
> Thank you.
>
> > I think the change you are suggesting is to improve
> > performance for optimization disabled builds by
> removing
> > an extra call. Is that correct?
> >
>
> Well, for DEBUG builds, yes. But given that the function
> call cannot
> be optimized away (on non-LTO builds), it affects
> optimized builds as
> well.
Do you mean compiler optimizations enabled, but linker
optimizations disabled.
>
> --
> Ard.
>
>
> >> -----Original Message-----
> >> From: Ard Biesheuvel
> [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Thursday, December 7, 2017 9:43 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>
> >> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
> >> devel@lists.01.org; Gao, Liming
> <liming.gao@intel.com>;
> >> Leif Lindholm <leif.lindholm@linaro.org>
> >> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
> >> conditions in ASSERT_[EFI|RETURN]_ERROR
> >>
> >> On 7 December 2017 at 17:36, Kinney, Michael D
> >> <michael.d.kinney@intel.com> wrote:
> >> > Ard,
> >> >
> >> > With link time optimization, the current order
> produces
> >> > smaller code.
> >> >
> >>
> >> I don't think it does. You are essentially saying that
> >> DebugAssertEnabled() may resolve to a link time
> constant
> >> FALSE under
> >> LTO.
> >>
> >> In that case, why would the following two statement
> not
> >> be equivalent?
> >>
> >> if (FALSE && EFI_ERROR (StatusParameter)) {}
> >>
> >> if (EFI_ERROR (StatusParameter) && FALSE) {}
> >>
> >> (which is essentially what a nested if () resolves to)
> >>
> >> In other words, the compiler is smart enough to drop
> the
> >> status check
> >> in the second case, because it can see there are no
> side
> >> effects, and
> >> the condition can never be made true anyway.
> >>
> >> > Without link time optimization, your patch will
> produce
> >> > smaller code, but not as small as link time
> optimized
> >> code.
> >> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR
2017-12-07 20:33 ` Kinney, Michael D
@ 2017-12-07 20:42 ` Ard Biesheuvel
0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-12-07 20:42 UTC (permalink / raw)
To: Kinney, Michael D
Cc: Alexei Fedorov, edk2-devel@lists.01.org, Leif Lindholm,
Gao, Liming
On 7 December 2017 at 20:33, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
>> On Behalf Of Ard Biesheuvel
>> Sent: Thursday, December 7, 2017 11:53 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; edk2-
>> devel@lists.01.org; Leif Lindholm
>> <leif.lindholm@linaro.org>; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: Re: [edk2] [PATCH] MdePkg/DebugLib; swap if
>> conditions in ASSERT_[EFI|RETURN]_ERROR
>>
>> On 7 December 2017 at 19:49, Kinney, Michael D
>> <michael.d.kinney@intel.com> wrote:
>> > Ard,
>> >
>> > I do not disagree with your logic.
>> >
>> > The current algorithm is based on data from a long
>> > time ago using what are now very old tool chains.
>> >
>>
>> With LTO?
>
> Yes. The LTCG feature for VS tool chains.
>
>>
>> > I will do some experiments on the currently supported
>> > toolchains to see if the optimization is the same
>> either
>> > way.
>> >
>>
>> Thank you.
>>
>> > I think the change you are suggesting is to improve
>> > performance for optimization disabled builds by
>> removing
>> > an extra call. Is that correct?
>> >
>>
>> Well, for DEBUG builds, yes. But given that the function
>> call cannot
>> be optimized away (on non-LTO builds), it affects
>> optimized builds as
>> well.
>
> Do you mean compiler optimizations enabled, but linker
> optimizations disabled.
>
Basically, yes. LTO has only been added recently for GCC5 on
ARM/AARCH64, and we are currently adding support for CLANG38 as well.
CLANG35 and RVCT do not support LTO.
So non-LTO still needs to be supported as well, and in some
debugging/tracing contexts, having lots of needless function calls is
making our lives difficult. (Hence my additional comment regarding
ASSERT (), although I suppose in some cases, calculating the result of
the expression could be more costly than the actual function call)
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-07 20:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-07 15:12 [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR Ard Biesheuvel
2017-12-07 15:26 ` Ard Biesheuvel
2017-12-07 17:01 ` Kinney, Michael D
2017-12-07 17:09 ` Ard Biesheuvel
2017-12-07 17:13 ` Ard Biesheuvel
2017-12-07 17:36 ` Kinney, Michael D
2017-12-07 17:43 ` Ard Biesheuvel
2017-12-07 19:49 ` Kinney, Michael D
2017-12-07 19:52 ` Ard Biesheuvel
2017-12-07 20:33 ` Kinney, Michael D
2017-12-07 20:42 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox