Re: [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

["Gao, Liming" <liming.gao@intel.com>]

Andrew:
  BZ 1163 is to remove inline X86 assembly code in C source file. But, this patch is wrong. I have gave my comments to update this patch.

  The change is to reduce the duplicated implementation. It will be good on the code maintain. Recently, one patch has to update .c and .nasm both. Please see https://lists.01.org/pipermail/edk2-devel/2019-February/037165.html. Beside this change, I propose to remove all GAS assembly code for IA32 and X64 arch. After those change, the patch owner will collect the impact of the image size. 

Thanks
Liming
> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Tuesday, March 5, 2019 12:58 PM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code
> 
> Shenglei,
> 
> I was confused by the names of these patches. From a C point of view this is inline assembly:
> 
> VOID
> EFIAPI
> CpuBreakpoint (
> VOID
> )
> {
> __asm__ __volatile__ ("int $3");
> }
> 
> These patches seem to be removing GAS and MASM assembly files, but not the inline assembly *.c files?
> 
> We don't want to remove the inline assembly from the BaseLib as that could have size, performance, and compiler optimization impact.
> 
> For example CpuBreakpoint() for clang with LTO would end up inlining a single byte. For i385 a call to assembler would be 5 bytes at each
> call location plus the overhead of the function. So that is a size increase and a performance overhead. For a C complier calling an assembly
> function is a serializing event an that can restrict optimizations. Thus having some limited inline assembly support is very useful.
> 
> Thanks,
> 
> Andrew Fish
> 
> > On Mar 4, 2019, at 5:40 PM, Shenglei Zhang <shenglei.zhang@intel.com> wrote:
> >
> > MdePkg BaseSynchronizationLib still uses the inline X86 assembly code
> > in C code files.It should be updated to consume nasm only.
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1163
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> > .../Library/BaseSynchronizationLib/BaseSynchronizationLib.inf   | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> > index 32414b29fa..719dc1938d 100755
> > --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> > +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> > @@ -75,13 +75,11 @@
> >
> > [Sources.ARM]
> > Synchronization.c
> > -  Arm/Synchronization.asm       | RVCT
> > Arm/Synchronization.S         | GCC
> >
> > [Sources.AARCH64]
> > Synchronization.c
> > AArch64/Synchronization.S     | GCC
> > -  AArch64/Synchronization.asm   | MSFT
> >
> > [Packages]
> > MdePkg/MdePkg.dec
> > --
> > 2.18.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel