Re: [Patch 4/4] BaseTools Makefile: Enable Ofast option for GCC tool chain

[Laszlo Ersek <lersek@redhat.com>]

Hi Liming,

On 09/29/16 16:12, Liming Gao wrote:
> Enable Ofast option to generate fast code for performance improvement.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
>  BaseTools/Source/C/Makefiles/header.makefile | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Makefiles/header.makefile b/BaseTools/Source/C/Makefiles/header.makefile
> index f2041f8..ca2dc2e 100644
> --- a/BaseTools/Source/C/Makefiles/header.makefile
> +++ b/BaseTools/Source/C/Makefiles/header.makefile
> @@ -44,12 +44,12 @@ ARCH_INCLUDE = -I $(MAKEROOT)/Include/AArch64/
>  endif
>  
>  INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I $(MAKEROOT)/Include/Common -I $(MAKEROOT)/Include/ -I $(MAKEROOT)/Include/IndustryStandard -I $(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE) 
> -BUILD_CPPFLAGS = $(INCLUDE)
> +BUILD_CPPFLAGS = $(INCLUDE) -Ofast
>  ifeq ($(DARWIN),Darwin)
>  # assume clang or clang compatible flags on OS X
> -BUILD_CFLAGS = -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations -Wno-self-assign -nostdlib -c -g
> +BUILD_CFLAGS = -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations -Wno-self-assign -Wno-unused-result -nostdlib -c -g
>  else
> -BUILD_CFLAGS = -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations -nostdlib -c -g
> +BUILD_CFLAGS = -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations -Wno-unused-result -nostdlib -c -g
>  endif
>  BUILD_LFLAGS =
>  BUILD_CXXFLAGS =
> 

are you sure -Ofast is a good idea? The gcc manual says,

  -Ofast
    Disregard strict standards compliance.  -Ofast enables all -O3
    optimizations.  It also enables optimizations that are not valid for
    all standard-compliant programs.  It turns on -ffast-math and the
    Fortran-specific -fno-protect-parens and -fstack-arrays.

To me this sounds quite scary -- I'm worried it might break the base
utilities in various obscure ways, which in turn could break the
firmware built with these tools in obscure ways.

Most upstream projects and GNU/Linux distributions use -O2 for
performance-optimized builds. Can you try that please and see if the
performance improvements (relative to the current status) are still
acceptable? If so, I would strongly prefer -O2 over -Ofast.

Also, while building BaseTools with your series applied (using gcc-4.8),
I saw one warning issued:

> g++ -c -I Pccts/h -I .. -I ../Include/Common -I ../Include/ -I ../Include/IndustryStandard -I ../Common/ -I .. -I . -I ../Include/X64/  -Ofast  VfrFormPkg.cpp -o VfrFormPkg.o
> VfrFormPkg.cpp: In member function 'void CIfrRecordInfoDB::IfrUpdateRecordInfoForDynamicOpcode(BOOLEAN)':
> VfrFormPkg.cpp:1360:91: warning: deprecated conversion from string constant to 'CHAR8* {aka char*}' [-Wwrite-strings]
>      gCVfrErrorHandle.PrintMsg (0, "Error", "Can not find the adjust offset in the record.");
                                                                                           ^
Thanks!
Laszlo