On 7 December 2017 at 16:53, Alexei Fedorov <[email protected]> wrote: > As expected with new ASSERT_EFI_ERROR () definition compiler generates 1 > conditional branch at the start: > > > // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: > ASSERT_EFI_ERROR (Status); > .loc 1 79 0 > tbz x0, #63, .L4 // Status, >
Thanks for confirming! > > ________________________________ > From: Ard Biesheuvel <[email protected]> > Sent: 07 December 2017 15:10:05 > To: Alexei Fedorov > Cc: Evan Lloyd; [email protected]; [email protected]@arm.com; > [email protected]@arm.com; [email protected]@arm.com; > [email protected]@arm.com > Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug > asserts > > 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. > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
