Change in coreboot[master]: [TEST] Add support for link time optimization
Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... [TEST] Add support for link time optimization - Code generation is not done until after linking, so the compiler must be invoked at all linking stages instead of the linker. As a consequence all linker arguments must be prefixed with -Wl. - Partial linking is not supported. Instead, object files are collected into thin archives that are linked instead. - The dead_code() macro causes linking errors, since dead functions aren't optimized out until after linking has begun. This macro could be replaced with the preprocessor if necessary, or just disabled for LTO builds. - Wrapping libgcc functions causes a symbol mismatch when using LTO. Wrapping these functions was originally done to support alternate regparam values, though AFAICT this isn't used anywhere. Using LTO leads to a ~10% decrease in stage size for QEMU and ~18% for the Thinkpad T500, and both targets boot successfully. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/include/assert.h M src/lib/Makefile.inc M src/lib/gcc.c M toolchain.inc M util/xcompile/xcompile 9 files changed, 42 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/1 diff --git a/Makefile.inc b/Makefile.inc index 1f18726..b85c11b 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -434,6 +434,10 @@ endif endif +ifeq ($(CONFIG_LTO),y) +CFLAGS_common += -flto -fuse-linker-plugin -fno-fat-lto-objects +endif + ADAFLAGS_common += -gnatp ADAFLAGS_common += -Wuninitialized -Wall -Werror ADAFLAGS_common += -pipe -g -nostdinc @@ -478,7 +482,7 @@ # Disable style checks for now ADAFLAGS_common += -gnatyN -LDFLAGS_common := --gc-sections -nostdlib -nostartfiles -static --emit-relocs +LDFLAGS_common := -nostdlib -nostartfiles -static -Wl,--emit-relocs,--gc-sections ifeq ($(CONFIG_WARNINGS_ARE_ERRORS),y) CFLAGS_common += -Werror diff --git a/src/Kconfig b/src/Kconfig index f75f942..4b81818 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -116,6 +116,15 @@ For details see https://ccache.samba.org. +config LTO + bool "Use link time optimization" + # Enable now for testing + default y + depends on COMPILER_GCC + help + Compile with link time optimization. This can often decrease the + final binary size, but may increase compilation time. + config FMD_GENPARSER bool "Generate flashmap descriptor parser using flex and bison" default n diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 534f2ce..75f96d2 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -80,7 +80,7 @@ $$(objcbfs)/$(1).debug: $$$$($(1)-libs) $$$$($(1)-objs) @printf " LINK $$(subst $$(obj)/,,$$(@))\n" - $$(LD_$(1)) $$(LDFLAGS_$(1)) -o $$@ -L$$(obj) $$(COMPILER_RT_FLAGS_$(1)) --whole-archive --start-group $$(filter-out %.ld,$$($(1)-objs)) $$($(1)-libs) --no-whole-archive $$(COMPILER_RT_$(1)) --end-group -T $(call src-to-obj,$(1),$(dir)/memlayout.ld) --oformat $(2) + $$(LD_$(1)) $$(LDFLAGS_$(1)) -o $$@ -L$$(obj) $$(COMPILER_RT_FLAGS_$(1)) -Wl,--whole-archive,--start-group $$(filter-out %.ld,$$($(1)-objs)) $$($(1)-libs) -Wl,--no-whole-archive $$(COMPILER_RT_$(1)) -Wl,--end-group -T $(call src-to-obj,$(1),$(dir)/memlayout.ld) -Wl,--oformat=$(2) -LANG=C LC_ALL= $$(OBJCOPY_$(1)) --only-section .illegal_globals $$(@) $$(objcbfs)/$(1)_null.offenders >/dev/null 2>&1 if [ -z "$$$$($$(NM_$(1)) $$(objcbfs)/$(1)_null.offenders 2>&1 | grep 'no symbols')" ];then \ echo "Forbidden global variables in $(1):"; \ @@ -212,11 +212,11 @@ postcar-y += postcar.c postcar-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c -LDFLAGS_postcar += -Map $(objcbfs)/postcar.map +LDFLAGS_postcar += -Wl,-Map,$(objcbfs)/postcar.map $(objcbfs)/postcar.debug: $$(postcar-objs) @printf " LINK $(subst $(obj)/,,$(@))\n" - $(LD_postcar) $(LDFLAGS_postcar) -o $@ -L$(obj) $(COMPILER_RT_FLAGS_postcar) --whole-archive --start-group $(filter-out %.ld,$^) --no-whole-archive $(COMPILER_RT_postcar) --end-group -T $(call src-to-obj,postcar,src/arch/x86/memlayout.ld) + $(LD_postcar) $(LDFLAGS_postcar) -o $@ -L$(obj) $(COMPILER_RT_FLAGS_postcar) -Wl,--whole-archive,--start-group $(filter-out %.ld,$^) -Wl,--no-whole-archive $(COMPILER_RT_postcar) -Wl,--end-group -T $(call src-to-obj,postcar,src/arch/x86/memlayout.ld) $(objcbfs)/postcar.elf: $(objcbfs)/postcar.debug.rmod cp $< $@ @@ -309,18 +309,20 @@ endif -$(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.o $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) - @printf " CC $(subst $(obj)/,,$(@))\n" - $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) -o $@ -L$(obj) $< -T $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) - -$(objgenerated)/ramstage.o: $$(ramstage-objs) $(COMPILER_RT_ramstage) $$(ramstage-libs) - @printf " CC $(subst $(obj)/,,$(@))\n" ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) - $(LD_ramstage) -m elf_i386 -r -o $@ $(COMPILER_RT_FLAGS_ramstage) --whole-archive --start-group $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) --no-whole-archive $(COMPILER_RT_ramstage) --end-group +LDFLAGS_ramstage += -Wl,-m,elf_i386 else - $(LD_ramstage) -m elf_x86_64 -r -o $@ $(COMPILER_RT_FLAGS_ramstage) --whole-archive --start-group $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) --no-whole-archive $(COMPILER_RT_ramstage) --end-group +LDFLAGS_ramstage += -Wl,-m,elf_x86_64 endif +$(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.a $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) + @printf " CC $(subst $(obj)/,,$(@))\n" + $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) $(COMPILER_RT_FLAGS_ramstage) -o $@ -L$(obj) -Wl,--whole-archive,--start-group $< -Wl,--no-whole-archive $(COMPILER_RT_ramstage) -Wl,--end-group -T $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) + +$(objgenerated)/ramstage.a: $$(ramstage-objs) $$(ramstage-libs) + @printf " AR $(subst $(obj)/,,$(@))\n" + $(AR_ramstage) rcT $@ $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) + endif # CONFIG_ARCH_RAMSTAGE_X86_32 / CONFIG_ARCH_RAMSTAGE_X86_64 smm-$(CONFIG_IDT_IN_EVERY_STAGE) += exception.c diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 11a4e67..f582a31 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -25,8 +25,8 @@ smm-generic-ccopts += -D__SMM__ smm-c-deps:=$$(OPTION_TABLE_H) -$(obj)/smm/smm.o: $$(smm-objs) $(COMPILER_RT_smm) - $(LD_smm) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smm) --whole-archive --start-group $(smm-objs) --no-whole-archive $(COMPILER_RT_smm) --end-group +$(obj)/smm/smm.a: $$(smm-objs) + $(AR_smm) rcT $@ $^ # change to the target path because objcopy will use the path name in its # ELF symbol names. @@ -53,7 +53,7 @@ # SMM Stub Module. The stub is used as a trampoline for relocation and normal # SMM handling. $(obj)/smmstub/smmstub.o: $$(smmstub-objs) $(COMPILER_RT_smmstub) - $(LD_smmstub) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smmstub) --whole-archive --start-group $(smmstub-objs) --no-whole-archive $(COMPILER_RT_smmstub) --end-group + $(LD_smmstub) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smmstub) -Wl,--whole-archive,--start-group $(smmstub-objs) -Wl,--no-whole-archive $(COMPILER_RT_smmstub) -Wl,--end-group # Link the SMM stub module with a 0-byte heap. ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) @@ -72,9 +72,9 @@ # C-based SMM handler. ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_32)) +$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.a, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_32)) else -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_64)) +$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.a, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_64)) endif $(obj)/smm/smm: $(obj)/smm/smm.elf.rmod @@ -82,8 +82,8 @@ else # CONFIG_SMM_TSEG -$(obj)/smm/smm: $(obj)/smm/smm.o $(src)/cpu/x86/smm/smm.ld - $(LD_smm) $(LDFLAGS_smm) -o $(obj)/smm/smm.elf -T $(src)/cpu/x86/smm/smm.ld $(obj)/smm/smm.o +$(obj)/smm/smm: $(obj)/smm/smm.a $(src)/cpu/x86/smm/smm.ld + $(LD_smm) $(LDFLAGS_smm) -o $(obj)/smm/smm.elf -T $(src)/cpu/x86/smm/smm.ld -Wl,--whole-archive,--start-group $(obj)/smm/smm.a -Wl,--no-whole-archive $(COMPILER_RT_smm) --end-group $(NM_smm) -n $(obj)/smm/smm.elf | sort > $(obj)/smm/smm.map $(OBJCOPY_smm) -O binary $(obj)/smm/smm.elf $@ diff --git a/src/include/assert.h b/src/include/assert.h index e0db0bc..6901953 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -61,7 +61,8 @@ dead_code_assertion_failed_at_line_##line(); \ } while (0) #define _dead_code(line) __dead_code(line) -#define dead_code() _dead_code(__LINE__) +//#define dead_code() _dead_code(__LINE__) +#define dead_code() /* This can be used in the context of an expression of type 'type'. */ #define dead_code_t(type) ({ \ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 2333f64..ac6d624 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -291,7 +291,7 @@ # rmdoule is named $(1).rmod define rmodule_link $(strip $(1)): $(strip $(2)) $$(COMPILER_RT_rmodules_$(4)) $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) | $$(RMODTOOL) - $$(LD_rmodules_$(4)) $$(LDFLAGS_rmodules_$(4)) $(RMODULE_LDFLAGS) -T $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) --defsym=__heap_size=$(strip $(3)) -o $$@ --whole-archive --start-group $(filter-out %.ld,$(2)) --end-group + $$(LD_rmodules_$(4)) $$(LDFLAGS_rmodules_$(4)) $(RMODULE_LDFLAGS) -T $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) -Wl,--defsym=__heap_size=$(strip $(3)) -o $$@ -Wl,--whole-archive,--start-group $(filter-out %.ld,$(2)) -Wl,--no-whole-archive $$(COMPILER_RT_rmodules_$(4)) -Wl,--end-group $$(NM_rmodules_$(4)) -n $$@ > $$(basename $$@).map endef diff --git a/src/lib/gcc.c b/src/lib/gcc.c index 5a93f45..66a7788 100644 --- a/src/lib/gcc.c +++ b/src/lib/gcc.c @@ -24,6 +24,7 @@ /* TODO: maybe this code should move to arch/x86 as architecture * specific implementations may vary */ +#if 0 #define WRAP_LIBGCC_CALL(type, name) \ asmlinkage type __real_##name(type a, type b); \ type __wrap_##name(type a, type b); \ @@ -33,3 +34,4 @@ WRAP_LIBGCC_CALL(unsigned long long, __udivdi3) WRAP_LIBGCC_CALL(long long, __moddi3) WRAP_LIBGCC_CALL(unsigned long long, __umoddi3) +#endif diff --git a/toolchain.inc b/toolchain.inc index 865227b..f52a518 100644 --- a/toolchain.inc +++ b/toolchain.inc @@ -117,7 +117,7 @@ $(error Check your .config file for CONFIG_ARCH_$(1)_* settings)) CC_$(1) := $(CC_$(2)) GCC_$(1) := $(GCC_CC_$(2)) -LD_$(1) := $(LD_$(2)) +LD_$(1) := $(CC_$(2)) NM_$(1) := $(NM_$(2)) AR_$(1) := $(AR_$(2)) GNATBIND_$(1) := $(GNATBIND_$(2)) @@ -130,7 +130,7 @@ CPPFLAGS_$(1) = $$(CPPFLAGS_common) $$(CPPFLAGS_$(2)) -D__ARCH_$(2)__ COMPILER_RT_$(1) := $$(COMPILER_RT_$(2)) COMPILER_RT_FLAGS_$(1) := $$(COMPILER_RT_FLAGS_$(2)) -LDFLAGS_$(1) = $$(LDFLAGS_common) $$(LDFLAGS_$(2)) +LDFLAGS_$(1) = $$(CFLAGS_$(1)) $$(LDFLAGS_common) $$(LDFLAGS_$(2)) endef # define_class: Allows defining any program as dynamic class and compiler tool diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile index 3203d71..8515813 100755 --- a/util/xcompile/xcompile +++ b/util/xcompile/xcompile @@ -363,7 +363,7 @@ TCLIST="i386 x86_64" TWIDTH="32" TABI="elf" - CC_RT_EXTRA_GCC="--wrap __divdi3 --wrap __udivdi3 --wrap __moddi3 --wrap __umoddi3" + #CC_RT_EXTRA_GCC="-Wl,--wrap=__divdi3,--wrap=__udivdi3,--wrap=__moddi3,--wrap=__umoddi3" } arch_config_ppc64() { -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: newchange
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 3: (2 comments) (How do you guys manage to comment on this without clearing the WIP flag? Gerrit always forces me to click "START REVIEW" when I'm trying to reply...) https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG@14 PS3, Line 14: - The dead_code() macro causes linking errors, since dead functions What exactly fails when you enable this? I took your patch, reverted the assert.h change and built GOOGLE_SCARLET (which uses that macro a bunch), and I didn't get any errors. I like that macro a lot and would like to keep it alive. It serves a function that isn't easily duplicated with other means (you cannot do the same thing with just the preprocessor without making a giant mess). https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig File src/Kconfig: https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig@123 PS2, Line 123: depends on COMPILER_GCC
Assuming the board builds and boots with Clang first, we would need to use the llvm-* versions of bi […] Isn't clang not working anyway (and I believe efforts to fix it have pretty much stopped)? I think we've decided to support GCC only for the foreseeable future.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 3 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 00:11:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jacob Garber <jgarber1@ualberta.ca> Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: comment
Hello ron minnich, build bot (Jenkins), Philipp Hug, Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38989 to look at the new patch set (#4). Change subject: [TEST] Add support for link time optimization ...................................................................... [TEST] Add support for link time optimization - Code generation is not done until after linking, so the compiler must be invoked at all linking stages instead of the linker. As a consequence all linker arguments must be prefixed with -Wl. - Partial linking is not supported. Instead, object files are collected into thin archives that are linked instead. - Wrapping libgcc functions causes a symbol mismatch when using LTO. Wrapping these functions was originally done to support alternate regparam values, though AFAICT this isn't used anywhere. Using LTO leads to a ~10% decrease in stage size for QEMU and ~18% for the Thinkpad T500, and both targets boot successfully. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig M src/arch/arm/Makefile.inc M src/arch/arm64/Makefile.inc M src/arch/ppc64/Makefile.inc M src/arch/riscv/Makefile.inc M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/lib/Makefile.inc M src/lib/gcc.c M toolchain.inc M util/xcompile/xcompile 12 files changed, 61 insertions(+), 44 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Julius Werner <jwerner@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG@14 PS3, Line 14: - The dead_code() macro causes linking errors, since dead functions
What exactly fails when you enable this? I took your patch, reverted the assert. […] I recall having some problems with it early on, but I can't reproduce them anymore. (They might have been triggered by the partial linking, which caused all sorts of strange errors.) I've reverted it for now and Jenkins seems happy, so it looks like it's not an issue.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 06:08:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 4:
Patch Set 3:
(2 comments)
(How do you guys manage to comment on this without clearing the WIP flag? Gerrit always forces me to click "START REVIEW" when I'm trying to reply...)
There is a great *SAVE* field on the bottom left. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 08:42:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig File src/Kconfig: https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig@123 PS2, Line 123: depends on COMPILER_GCC
Isn't clang not working anyway (and I believe efforts to fix it have pretty much stopped)? I think w […] I am able to successfully build and boot QEMU i440fx with LLVM/Clang.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 08:44:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Comment-In-Reply-To: Jacob Garber <jgarber1@ualberta.ca> Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: comment
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 4: http://dpaste.com/00PM90M the board boots with current patch -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 21 Feb 2020 15:51:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 4: Code-Review+1 (2 comments) https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig File src/Kconfig: https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig@120 PS4, Line 120: bool "Use link time optimization" Also add *(LTO)* at the end. https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig@122 PS4, Line 122: default y Could you split `default y` out to a separate commit please on top. That way the rest could be submitted to avoid bitrotting, and people could test more easily just turning this on in Kconfig. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 4 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Sun, 08 Mar 2020 08:56:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello build bot (Jenkins), Philipp Hug, Patrick Georgi, Martin Roth, Paul Menzel, Julius Werner, ron minnich, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38989 to look at the new patch set (#5). Change subject: [TEST] Add support for link time optimization ...................................................................... [TEST] Add support for link time optimization - Code generation is not done until after linking, so the compiler must be invoked at all linking stages instead of the linker. As a consequence all linker arguments must be prefixed with -Wl. - Partial linking is not supported. Instead, object files are collected into thin archives that are linked instead. Using LTO leads to a ~10% decrease in stage size for QEMU and ~18% for the Thinkpad T500, and both targets boot successfully. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig M src/arch/arm/Makefile.inc M src/arch/arm64/Makefile.inc M src/arch/ppc64/Makefile.inc M src/arch/riscv/Makefile.inc M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/lib/Makefile.inc M toolchain.inc M util/xcompile/xcompile 11 files changed, 59 insertions(+), 44 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 5 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-MessageType: newpatchset
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6: Any idea how to fix the error below for AGESA boards like ASRock E350M1? ``` LINK cbfs/fallback/romstage.debug /opt/xgcc/lib/gcc/i386-elf/8.3.0/../../../../i386-elf/bin/ld.bfd: section .illegal_globals VMA wraps around address space collect2: error: ld returned 1 exit status ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Sat, 25 Apr 2020 22:19:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6: I worked around that error with the hack below. ``` $ git diff diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 1615e75c9a..d818205ab7 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -78,11 +78,7 @@ $(1)-S-ccopts += -I. $$(objcbfs)/$(1).debug: $$$$($(1)-libs) $$$$($(1)-objs) @printf " LINK $$(subst $$(obj)/,,$$(@))\n" $$(LD_$(1)) $$(LDFLAGS_$(1)) -o $$@ -L$$(obj) $$(COMPILER_RT_FLAGS_$(1)) -Wl,--whole-archive,--start-group $$(filter-out %.ld,$$($(1)-objs)) $$($(1)-libs) -Wl,--no-whole-archive $$(COMPILER_RT_$(1)) -Wl,--end-group -T $(call src-to-obj,$(1),$(dir)/memlayout.ld) -Wl,--oformat=$(2) - -LANG=C LC_ALL= $$(OBJCOPY_$(1)) --only-section .illegal_globals $$(@) $$(objcbfs)/$(1)_null.offenders >/dev/null 2>&1 - if [ -z "$$$$($$(NM_$(1)) $$(objcbfs)/$(1)_null.offenders 2>&1 | grep 'no symbols')" ];then \ - echo "Forbidden global variables in $(1):"; \ - $$(NM_$(1)) $$(objcbfs)/$(1)_null.offenders; false; \ - fi + -LANG=C LC_ALL= $$(OBJCOPY_$(1)) --only-section $$(@) $$(objcbfs)/$(1)_null.offenders >/dev/null 2>&1 endef ############################################################################### diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 92b26a0877..05e317dd29 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -83,10 +83,6 @@ */ . = 0xffffff00; -.illegal_globals . : { - *(EXCLUDE_FILE ("*/libagesa.*.a:" "*/romstage*/buildOpts.o" "*/romstage*/agesawrapper.o" "*/vendorcode/amd/agesa/*" "*/vendorcode/amd/cimx/*") .data) - *(EXCLUDE_FILE ("*/libagesa.*.a:" "*/romstage*/buildOpts.o" "*/romstage*/agesawrapper.o" "*/vendorcode/amd/agesa/*" "*/vendorcode/amd/cimx/*") .data.*) -} _bogus = ASSERT((CONFIG_DCACHE_RAM_SIZE == 0) || (SIZEOF(.car.data) <= CONFIG_DCACHE_RAM_SIZE), "Cache as RAM area is too full"); #if CONFIG(PAGING_IN_CACHE_AS_RAM) ``` romstage is 9.3 % smaller, and ramstage around 11 %. ``` $ build/cbfstool build/coreboot.rom print FMAP REGION: COREBOOT Name Offset Type Size Comp cbfs master header 0x0 cbfs header 32 none fallback/romstage 0x80 stage 170260 none fallback/ramstage 0x29a00 stage 105668 none […] ``` No LTO: ``` fallback/romstage 0x80 stage 185012 none fallback/ramstage 0x2d3c0 stage 119027 none ``` With that, I reach the payload phase, but the drive connected to the SATA port is not detected by the payload (GRUB, SeaBIOS) and GNU/Linux (5.7-rc1 and 5.5) – also not with warm reboot. I cannot spot a difference in the coreboot log [1]. Booting from USB works. [1]: https://paste.flashrom.org/view.php?id=3306 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Sun, 26 Apr 2020 11:12:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6:
Patch Set 6:
I worked around that error with the hack below.
Yes, this is a current limitation of LTO right now. Because the object files are all lumped together into a single unit, all information about where the symbols came from is lost, so EXCLUDE_FILE is unable of excluding the AGESA objects from the illegal_globals check. Tracing where a symbol came from has been implemented in LLVM [0], but I'm not sure if it's on the roadmap for GCC. For now it's probably best to disable LTO when compiling AGESA. [0]: https://llvm.org/devmtg/2017-10/slides/LTOLinkerScriptsEdlerVonKoch.pdf -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Sun, 26 Apr 2020 19:20:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6: Keith, as you also seem to care about image size, could you please test, if this still works on your Intel 440BX boards? Size reduction for the asus/p2b (P2B-LS) seems also ten to 13 percent: 1. Without LTO: ``` fallback/romstage 0x80 stage 16412 none cpu_microcode_blob.bin 0x4100 microcode 83968 none fallback/ramstage 0x18980 stage 50347 none ``` 2. With LTO: ``` fallback/romstage 0x80 stage 14212 none cpu_microcode_blob.bin 0x3880 microcode 83968 none fallback/ramstage 0x18100 stage 44882 none ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Sun, 26 Apr 2020 21:50:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6: The build artifacts on the builders are gone, could you please rebase, so it’s tested again? -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Comment-Date: Tue, 28 Apr 2020 14:07:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6:
Keith, as you also seem to care about image size, could you please test, if this still works on your Intel 440BX boards?
Size reduction for the asus/p2b (P2B-LS) seems also ten to 13 percent:
1. Without LTO:
``` fallback/romstage 0x80 stage 16412 none cpu_microcode_blob.bin 0x4100 microcode 83968 none fallback/ramstage 0x18980 stage 50347 none ```
2. With LTO:
``` fallback/romstage 0x80 stage 14212 none cpu_microcode_blob.bin 0x3880 microcode 83968 none fallback/ramstage 0x18100 stage 44882 none ```
Here's my reduction results (in bytes) Bootblock: -936 (-23.54%) (effective code size, minus reset vector and build signature) romstage: -2176 (-13.38%) ramstage: -5343 (-10.84%) postcar: -908 (-9.07%) Now I can't directly make use of space saved in bootblock, but it gives us more room before we have to go to 16KiB bootblock. I'll now boot test it. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-Comment-Date: Tue, 28 Apr 2020 15:52:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... Patch Set 6:
Patch Set 6:
Keith, as you also seem to care about image size, could you please test, if this still works on your Intel 440BX boards?
I'll now boot test it.
I booted this patch on p2b-ls with no noticeable differences in functionalities. However I'm seeing corruption in the cbmem console output from earlier boots. Unsure what the cause is, as the test setup is bare with no physical support of any kind. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 6 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-Comment-Date: Tue, 28 Apr 2020 17:02:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Keith Hui, build bot (Jenkins), Michał Żygowski, Philipp Hug, Patrick Georgi, Martin Roth, Paul Menzel, Julius Werner, ron minnich, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38989 to look at the new patch set (#7). Change subject: [TEST] Add support for link time optimization ...................................................................... [TEST] Add support for link time optimization Using LTO leads to a ~10% decrease in stage size for QEMU and ~18% for the Thinkpad T500, and both targets boot successfully. GCC currently does not track symbol origins when using LTO, so it must be disabled for AGESA so it can be excluded from the illegal_globals check. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig M src/vendorcode/amd/agesa/Makefile.inc 3 files changed, 19 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 7 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-MessageType: newpatchset
Hello Keith Hui, build bot (Jenkins), Michał Żygowski, Philipp Hug, Patrick Georgi, Martin Roth, Paul Menzel, Julius Werner, ron minnich, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38989 to look at the new patch set (#8). Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Kconfig,Makefile.inc: Add support for link time optimization Link time optimization is a technique for whole-program optimization. Instead of doing code generation during compilation, the compiler saves its intermediate representation to the object files. During the final linking step, it will then merge all the object files together and perform optimizations on the entire program. This can often reduce the final binary size, but also may increase the total compilation time. This patch introduces a Kconfig option for enabling link time optimization in coreboot. Due to the potential for breakage, this option is disabled by default, and would only be enabled per-board once it has been tested on hardware. Not all targets compile now, but early results are encouraging: using LTO leads to a ~13% decrease in stage size for QEMU i440fx and ~10% for the Thinkpad T500, and both targets boot successfully. Compile times for both are also about 10% faster. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig 2 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 8 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-MessageType: newpatchset
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 8: (5 comments) https://review.coreboot.org/c/coreboot/+/38989/2//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/38989/2//COMMIT_MSG@23 PS2, Line 23: the Thinkpad T500, and both targets boot successfully.
For the T500 it's ~8ms less, but otherwise about the same. […] Done
https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/38989/3//COMMIT_MSG@14 PS3, Line 14: - The dead_code() macro causes linking errors, since dead functions
I recall having some problems with it early on, but I can't reproduce them anymore. […] Done
https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig File src/Kconfig: https://review.coreboot.org/c/coreboot/+/38989/2/src/Kconfig@123 PS2, Line 123: depends on COMPILER_GCC
I am able to successfully build and boot QEMU i440fx with LLVM/Clang. Done
https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig File src/Kconfig: https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig@120 PS4, Line 120: bool "Use link time optimization"
Also add *(LTO)* at the end. Done
https://review.coreboot.org/c/coreboot/+/38989/4/src/Kconfig@122 PS4, Line 122: default y
Could you split `default y` out to a separate commit please on top. […] Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 8 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-Comment-Date: Tue, 28 Apr 2020 23:03:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Comment-In-Reply-To: Jacob Garber <jgarber1@ualberta.ca> Gerrit-MessageType: comment
Hello Keith Hui, build bot (Jenkins), Michał Żygowski, Philipp Hug, Patrick Georgi, Martin Roth, Paul Menzel, Julius Werner, ron minnich, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38989 to look at the new patch set (#9). Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Kconfig,Makefile.inc: Add support for link time optimization Link time optimization is a technique for whole-program optimization. Instead of doing code generation during compilation, the compiler saves its intermediate representation to the object files. During the final linking step, it will then merge all the object files together and perform optimizations on the entire program. This can often reduce the final binary size, but also may increase the total compilation time. This patch introduces a Kconfig option for enabling link time optimization in coreboot. Due to the potential for breakage, this option is disabled by default, and would only be enabled per-board once it has been tested on hardware. Not all targets compile now, but early results are encouraging: using LTO leads to a ~13% decrease in stage size for QEMU i440fx and ~10% for the Thinkpad T500, and both targets boot successfully. Compile times for both are also about 10% faster. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.inc M src/Kconfig 2 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 9 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-MessageType: newpatchset
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 10: Jacob, maybe we need to rebase 42251 on current patch ? Please feel free if you want to do so.... -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 10 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-Comment-Date: Fri, 28 Aug 2020 15:55:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11: it doesn't work here, what am I doing wrong? `build/bootblock/lib/libgcc.o: plugin needed to handle lto object` -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Mon, 31 Aug 2020 06:16:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11:
Patch Set 11:
it doesn't work here, what am I doing wrong? `build/bootblock/lib/libgcc.o: plugin needed to handle lto object`
What board do you build for? -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Mon, 31 Aug 2020 07:16:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11:
Patch Set 11:
Patch Set 11:
it doesn't work here, what am I doing wrong? `build/bootblock/lib/libgcc.o: plugin needed to handle lto object`
What board do you build for?
Qemu i440fx -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Mon, 31 Aug 2020 07:18:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11:
Patch Set 11:
it doesn't work here, what am I doing wrong? `build/bootblock/lib/libgcc.o: plugin needed to handle lto object`
Are you doing a regular build? Some of the tools need to be changed around to be LTO-aware (such as CB:40811), though it should work if all the patches in this train are applied. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Tue, 01 Sep 2020 22:09:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11: Code-Review+1 tested successfully for system76/lemp9. builds fine, boots fine -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Tue, 15 Sep 2020 20:10:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11:
Patch Set 11: Code-Review+1
tested successfully for system76/lemp9. builds fine, boots fine
That's good to hear, I'm glad there isn't any breakage. Did the binary size decrease at all? -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Wed, 16 Sep 2020 01:46:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 11:
Patch Set 11:
Patch Set 11: Code-Review+1
tested successfully for system76/lemp9. builds fine, boots fine
That's good to hear, I'm glad there isn't any breakage. Did the binary size decrease at all?
LTO disabled: fallback/romstage 0x80 stage 47188 none fallback/ramstage 0x3b1c0 stage 106256 none fallback/postcar 0x116680 stage 20252 none fallback/payload 0x11b600 simple elf 461468 none bootblock 0xba3dc0 bootblock 49152 none LTO enabled: fallback/romstage 0x80 stage 42876 none - 9.13 % fallback/ramstage 0x3a100 stage 94121 none -11.42 % fallback/postcar 0x112680 stage 18348 none - 9.40 % fallback/payload 0x116e80 simple elf 461651 none + 0.04 % bootblock 0xba3dc0 bootblock 49152 none 0.00 % -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 11 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Thu, 17 Sep 2020 08:00:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 12: One thing that worked before for me, doesn't work anymore. I'm not sure if there's any solution for this. When testing tiny changes, I never did a make clean but just ran `make -j5` to rebuild, where Make just rebuilds the changed source and resolved dependencies automagically. With the LTO patches, this does not seem to work anymore. The final linking step fails: https://paste.xinu.at/gOLYHtjrF4SO68Vam0HfecWreiN4ZJyP8k8GwRk4HTRSfV25v0Cd4u... Any ideas? -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 12 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Thu, 29 Oct 2020 18:40:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 12:
Patch Set 12:
One thing that worked before for me, doesn't work anymore. I'm not sure if there's any solution for this.
When testing tiny changes, I never did a make clean but just ran `make -j5` to rebuild, where Make just rebuilds the changed source and resolved dependencies automagically. With the LTO patches, this does not seem to work anymore. The final linking step fails: https://paste.xinu.at/gOLYHtjrF4SO68Vam0HfecWreiN4ZJyP8k8GwRk4HTRSfV25v0Cd4u...
Any ideas?
Good catch. The problem is the previous commit that replaces partial linking with thin archives. Trying to find a fix now. -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 12 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Fri, 30 Oct 2020 03:43:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Jacob Garber. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 13: (1 comment) Patchset: PS13: ramstage is now 2 MB in size. ``` CBFS fallback/romstage E: Input file size (2135728) greater than page size (261632). E: 'fallback/romstage' can't fit in CBFS for page-size 0, align 0x40. E: Failed while operating on 'COREBOOT' region! E: The image will be left unmodified. make[1]: *** [Makefile.inc:1101: /cb-build/coreboot-gerrit.0/default/EMULATION_QEMU_X86_I440FX_X86_64/coreboot.pre] Error 1 make[1]: Leaving directory '/home/coreboot/node-root/workspace/coreboot-gerrit' ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 13 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: Ron Minnich <rminnich@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Attention: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Comment-Date: Tue, 25 May 2021 12:03:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Jacob Garber. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 15: Code-Review+1 (1 comment) Patchset: PS15:
Patch Set 12:
Patch Set 12:
One thing that worked before for me, doesn't work anymore. I'm not sure if there's any solution for this.
When testing tiny changes, I never did a make clean but just ran `make -j5` to rebuild, where Make just rebuilds the changed source and resolved dependencies automagically. With the LTO patches, this does not seem to work anymore. The final linking step fails: https://paste.xinu.at/gOLYHtjrF4SO68Vam0HfecWreiN4ZJyP8k8GwRk4HTRSfV25v0Cd4u...
Any ideas?
Good catch. The problem is the previous commit that replaces partial linking with thin archives. Trying to find a fix now.
Have you ever found a solution for this? -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 15 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth - Personal <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: Ron Minnich <rminnich@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Raul Rangel <rrangel@chromium.org> Gerrit-Attention: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Comment-Date: Sun, 09 Jan 2022 01:54:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Attention is currently required from: Michael Niewöhner. Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 15: (1 comment) Patchset: PS15:
Patch Set 12: […] Oh yes I did. I forget the exact problem but it was something with the archives not being regenerated in one particular case, it should be fixed now.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 15 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin Roth - Personal <martinroth@google.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: Ron Minnich <rminnich@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Raul Rangel <rrangel@chromium.org> Gerrit-Attention: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Comment-Date: Sun, 09 Jan 2022 02:37:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michael Niewöhner <foss@mniewoehner.de> Gerrit-MessageType: comment
Attention is currently required from: Jacob Garber, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Patch Set 15: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/38989/comment/6ed47765_7096aadf PS15, Line 19: Not all targets compile now Could we add a config file to build-test at least one of the targets that works? https://review.coreboot.org/c/coreboot/+/38989/comment/351c9ff7_89f7b5f4 PS15, Line 21: successfully. nit: move to next line -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 15 Gerrit-Owner: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Raul Rangel <rrangel@chromium.org> Gerrit-Attention: Jacob Garber <jgarber1@ualberta.ca> Gerrit-Attention: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Comment-Date: Wed, 02 Nov 2022 17:50:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Attention is currently required from: Jacob Garber, Michael Niewöhner. Arthur Heymans has uploaded a new patch set (#16) to the change originally created by Jacob Garber. ( https://review.coreboot.org/c/coreboot/+/38989?usp=email ) Change subject: Kconfig,Makefile.inc: Add support for link time optimization ...................................................................... Kconfig,Makefile.inc: Add support for link time optimization Link time optimization is a technique for whole-program optimization. Instead of doing code generation during compilation, the compiler saves its intermediate representation to the object files. During the final linking step, it will then merge all the object files together and perform optimizations on the entire program. This can often reduce the final binary size, but also may increase the total compilation time. This patch introduces a Kconfig option for enabling link time optimization in coreboot. Due to the potential for breakage, this option is disabled by default, and would only be enabled per-board once it has been tested on hardware. Not all targets compile now, but early results are encouraging: using LTO leads to a ~13% decrease in stage size for QEMU i440fx and ~10% for the Thinkpad T500, and both targets boot successfully. Compile times for both are also about 10% faster. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> --- M Makefile.mk M src/Kconfig 2 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/16 -- To view, visit https://review.coreboot.org/c/coreboot/+/38989?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 16 Gerrit-Owner: Jacob Garber <jacob@jwgarber.ca> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Jacob Garber <jacob@jwgarber.ca> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Keith Hui <buurin@gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss@mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org> Gerrit-Reviewer: Philipp Hug <philipp@hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Keith Hui <buurin@gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Raul Rangel <rrangel@chromium.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Attention: Jacob Garber <jacob@jwgarber.ca> Gerrit-Attention: Michael Niewöhner <foss@mniewoehner.de> Gerrit-MessageType: newpatchset
participants (9)
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Julius Werner (Code Review)
-
Keith Hui (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)