On 7 December 2017 at 14:55, Alexei Fedorov <[email protected]> wrote:
> Hi,
>
> I've compiled current HdLcd.c code with different optimisation levels for
> DEBUG build using GCC 7.2.1, and the assembler code below was generated for:
>
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR( Status )) {
> return EFI_DEVICE_ERROR;
> }
>
> -O0 (default DEBUG option for AARCH64 before Ard's patch):
>
>
> str x0, [x29, 72] //, Status
>
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
> .loc 1 79 0
> bl DebugAssertEnabled //
> and w0, w0, 255 // _1, tmp150
> cmp w0, 0 // _1,
> beq .L4 //,
> ldr x0, [x29, 72] // Status.9_2, Status
> cmp x0, 0 // Status.9_2,
> bge .L4 //,
>
> 2. -Os:
>
> mov x19, x0 // Status,
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
> .loc 1 79 0
> bl DebugAssertEnabled //
> .LVL15:
> tst w0, 255 //
> beq .L4 //,
> tbz x19, #63, .L8 // Status,
>
> 3. -O3:
>
> mov x19, x0 // Status,
> .LVL14:
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
> bl DebugAssertEnabled //
> .LVL15:
> tst w0, 255 //
> beq .L5 //,
> tbnz x19, #63, .L26 // Status,
>
> with DebugAssertEnabled() compiled as:
>
> DebugAssertEnabled:
> .LFB4:
> .loc 1 203 0
> .cfi_startproc
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204: return
> (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0);
> .loc 1 204 0
> adrp x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask // tmp80,
> add x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask //
> tmp79, tmp80,
> ldrb w0, [x0] // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1,
> _gPcd_FixedAtBuild_PcdDebugPropertyMask
> and w0, w0, 1 // _3, _2,
> cmp w0, 0 // _3,
> cset w0, ne // tmp82,
> and w0, w0, 255 // _4, tmp81
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: }
> .loc 1 205 0
> ret
>
> As you can see each "ASSERT_EFI_ERROR (Status)" macro requires
> DebugAssertEnabled() call taking 8 instructions by itself + minimum 3
> instructions (for -Os, -O3) for Status check, which will be performed by "if
> (EFI_ERROR( Status ))" anyway.
> Please correct me if I'm wrong assuming that placing
> ASSERT_EFI_ERROR (Status)
> inside
> if (EFI_ERROR( Status )) {
> statement is an optimisation improvement.
>
Thanks for digging this up. This appears to be an oversight in the
definition of the ASSERT_EFI_ERROR () macro, which is currently
defined as
#if !defined(MDEPKG_NDEBUG)
#define ASSERT_EFI_ERROR(StatusParameter) \
do { \
if (DebugAssertEnabled ()) { \
if (EFI_ERROR (StatusParameter)) { \
which is obviously the wrong way around, given that the compiler can
never optimize away the function call, due to its potential side
effects (which it doesn't have)
So I will propose to fix this, by swapping the two if () statements.
Please let me know if the issue is still reproducible with that patch
applied.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
