| Message ID | 20210303041211.26945-2-marek.behun@nic.cz |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | U-Boot LTO (Sandbox + ARM Nokia RX-51) | expand |
Hi Marek, On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > Using thin archives instead of incremental linking > - saves disk space > - works better with dead code elimination > - prepares for potential LTO The commit message is a little bit confusing. This commit actually does 2 things: don't do incremental linking (using --whole-archive), and use thin archive (passing T to ar). I believe they are for different purposes, so we cannot say "using thin archives instead of incremental linking". > > Linux does this for some time now, do this also in U-Boot. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > --- > Makefile | 2 +- > arch/sandbox/config.mk | 6 +++--- > scripts/Makefile.build | 5 ++--- > scripts/Makefile.spl | 7 +++---- > 4 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/Makefile b/Makefile > index 6cdd3677eb..33d0b80de8 100644 > --- a/Makefile > +++ b/Makefile > @@ -1750,7 +1750,7 @@ ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(ARCH)/Makefile.postlink) > quiet_cmd_u-boot__ ?= LD $@ > cmd_u-boot__ ?= $(LD) $(KBUILD_LDFLAGS) $(LDFLAGS_u-boot) -o $@ \ > -T u-boot.lds $(u-boot-init) \ > - --start-group $(u-boot-main) --end-group \ > + --whole-archive $(u-boot-main) --no-whole-archive \ > $(PLATFORM_LIBS) -Map u-boot.map; \ > $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true) > > diff --git a/arch/sandbox/config.mk b/arch/sandbox/config.mk > index 189e9c2b0c..ebbb094744 100644 > --- a/arch/sandbox/config.mk > +++ b/arch/sandbox/config.mk > @@ -17,13 +17,13 @@ PLATFORM_CPPFLAGS += $(shell $(SDL_CONFIG) --cflags) > endif > > cmd_u-boot__ = $(CC) -o $@ -Wl,-T u-boot.lds $(u-boot-init) \ > - -Wl,--start-group $(u-boot-main) -Wl,--end-group \ > + -Wl,--whole-archive $(u-boot-main) -Wl,--no-whole-archive \ > $(PLATFORM_LIBS) -Wl,-Map -Wl,u-boot.map > > cmd_u-boot-spl = (cd $(obj) && $(CC) -o $(SPL_BIN) -Wl,-T u-boot-spl.lds \ > $(patsubst $(obj)/%,%,$(u-boot-spl-init)) \ > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ u-boot-spl-platdata is still within --start-group, --end-group, is this intentional? > $(PLATFORM_LIBS) -Wl,-Map -Wl,u-boot-spl.map -Wl,--gc-sections) > > CONFIG_ARCH_DEVICE_TREE := sandbox > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 705a886cb9..3659d0af1b 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -331,11 +331,10 @@ $(sort $(subdir-obj-y)): $(subdir-ym) ; > # Rule to compile a set of .o files into one .o file > # > ifdef builtin-target > -quiet_cmd_link_o_target = LD $@ > +quiet_cmd_link_o_target = AR $@ > # If the list of objects to link is empty, just create an empty built-in.o > cmd_link_o_target = $(if $(strip $(obj-y)),\ > - $(LD) $(ld_flags) -r -o $@ $(filter $(obj-y), $^) \ > - $(cmd_secanalysis),\ > + rm -f $@; $(AR) cDPrsT $@ $(filter $(obj-y), $^), \ Is P required to make everything work? > rm -f $@; $(AR) rcs$(KBUILD_ARFLAGS) $@) > > $(builtin-target): $(obj-y) FORCE > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > index ea4e045769..f9faf804de 100644 > --- a/scripts/Makefile.spl > +++ b/scripts/Makefile.spl > @@ -421,10 +421,9 @@ $(obj)/$(SPL_BIN).sym: $(obj)/$(SPL_BIN) FORCE > # May be overridden by arch/$(ARCH)/config.mk > quiet_cmd_u-boot-spl ?= LD $@ > cmd_u-boot-spl ?= (cd $(obj) && $(LD) $(KBUILD_LDFLAGS) $(LDFLAGS_$(@F)) \ > - $(patsubst $(obj)/%,%,$(u-boot-spl-init)) --start-group \ > - $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) \ > - --end-group \ > + $(patsubst $(obj)/%,%,$(u-boot-spl-init)) \ > + --whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) --no-whole-archive \ > + --start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) --end-group \ > $(PLATFORM_LIBS) -Map $(SPL_BIN).map -o $(SPL_BIN)) > > $(obj)/$(SPL_BIN): $(u-boot-spl-platdata) $(u-boot-spl-init) \ > -- Regards, Bin
On Thu, 4 Mar 2021 18:57:11 +0800 Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Marek, > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > > > Using thin archives instead of incremental linking > > - saves disk space > > - works better with dead code elimination > > - prepares for potential LTO > > The commit message is a little bit confusing. This commit actually > does 2 things: don't do incremental linking (using --whole-archive), > and use thin archive (passing T to ar). I believe they are for > different purposes, so we cannot say "using thin archives instead of > incremental linking". > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > u-boot-spl-platdata is still within --start-group, --end-group, is > this intentional? I confess that I did not really study these options, I have made these changes according to old LTO patches for Linux. But you are right that it does not make sense. I have fixed this for the next version of this patch. > Is P required to make everything work? It is not. Removed in next version.
Hi Marek, On Fri, Mar 5, 2021 at 2:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > On Thu, 4 Mar 2021 18:57:11 +0800 > Bin Meng <bmeng.cn@gmail.com> wrote: > > > Hi Marek, > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > > > > > Using thin archives instead of incremental linking > > > - saves disk space > > > - works better with dead code elimination > > > - prepares for potential LTO > > > > The commit message is a little bit confusing. This commit actually > > does 2 things: don't do incremental linking (using --whole-archive), > > and use thin archive (passing T to ar). I believe they are for > > different purposes, so we cannot say "using thin archives instead of > > incremental linking". > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > this intentional? > > I confess that I did not really study these options, I have made these > changes according to old LTO patches for Linux. But you are right that > it does not make sense. I have fixed this for the next version of this > patch. > > > Is P required to make everything work? > > It is not. Removed in next version. I did more investigation on this. The Linux kernel specially added P to ar, in below commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 So it looks like we should keep P here? But I don't get the point of switching to thin archives. Based on my experiment, LTO does not rely on thin archives. The Linux kernel did not introduce thin archives for LTO. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f Regards, Bin
On Fri, Mar 05, 2021 at 09:34:42PM +0800, Bin Meng wrote: > Hi Marek, > > On Fri, Mar 5, 2021 at 2:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Thu, 4 Mar 2021 18:57:11 +0800 > > Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > Hi Marek, > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > > > > > > > Using thin archives instead of incremental linking > > > > - saves disk space > > > > - works better with dead code elimination > > > > - prepares for potential LTO > > > > > > The commit message is a little bit confusing. This commit actually > > > does 2 things: don't do incremental linking (using --whole-archive), > > > and use thin archive (passing T to ar). I believe they are for > > > different purposes, so we cannot say "using thin archives instead of > > > incremental linking". > > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > > > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > > this intentional? > > > > I confess that I did not really study these options, I have made these > > changes according to old LTO patches for Linux. But you are right that > > it does not make sense. I have fixed this for the next version of this > > patch. > > > > > Is P required to make everything work? > > > > It is not. Removed in next version. > > I did more investigation on this. > > The Linux kernel specially added P to ar, in below commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 > > So it looks like we should keep P here? > > But I don't get the point of switching to thin archives. Based on my > experiment, LTO does not rely on thin archives. The Linux kernel did > not introduce thin archives for LTO. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f So technically it would just be part of dealing with the backlog of kbuild-resync to take it in this series I guess.
On Fri, 5 Mar 2021 21:34:42 +0800 Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Marek, > > On Fri, Mar 5, 2021 at 2:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Thu, 4 Mar 2021 18:57:11 +0800 > > Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > Hi Marek, > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > > > > > > > Using thin archives instead of incremental linking > > > > - saves disk space > > > > - works better with dead code elimination > > > > - prepares for potential LTO > > > > > > The commit message is a little bit confusing. This commit actually > > > does 2 things: don't do incremental linking (using --whole-archive), > > > and use thin archive (passing T to ar). I believe they are for > > > different purposes, so we cannot say "using thin archives instead of > > > incremental linking". > > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > > > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > > this intentional? > > > > I confess that I did not really study these options, I have made these > > changes according to old LTO patches for Linux. But you are right that > > it does not make sense. I have fixed this for the next version of this > > patch. > > > > > Is P required to make everything work? > > > > It is not. Removed in next version. > > I did more investigation on this. > > The Linux kernel specially added P to ar, in below commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 > > So it looks like we should keep P here? > > But I don't get the point of switching to thin archives. Based on my > experiment, LTO does not rely on thin archives. The Linux kernel did > not introduce thin archives for LTO. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f It does not matter whether we use thin archives or real archives. But we did not use any of this before, instead we linked the object files in a directory into one object file in that directory. And to do this with LTO would cause unnecessary complications. Marek
On Fri, 5 Mar 2021 08:37:28 -0500 Tom Rini <trini@konsulko.com> wrote: > On Fri, Mar 05, 2021 at 09:34:42PM +0800, Bin Meng wrote: > > Hi Marek, > > > > On Fri, Mar 5, 2021 at 2:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Thu, 4 Mar 2021 18:57:11 +0800 > > > Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > > Hi Marek, > > > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún <marek.behun@nic.cz> wrote: > > > > > > > > > > Using thin archives instead of incremental linking > > > > > - saves disk space > > > > > - works better with dead code elimination > > > > > - prepares for potential LTO > > > > > > > > The commit message is a little bit confusing. This commit actually > > > > does 2 things: don't do incremental linking (using --whole-archive), > > > > and use thin archive (passing T to ar). I believe they are for > > > > different purposes, so we cannot say "using thin archives instead of > > > > incremental linking". > > > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ > > > > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > > > this intentional? > > > > > > I confess that I did not really study these options, I have made these > > > changes according to old LTO patches for Linux. But you are right that > > > it does not make sense. I have fixed this for the next version of this > > > patch. > > > > > > > Is P required to make everything work? > > > > > > It is not. Removed in next version. > > > > I did more investigation on this. > > > > The Linux kernel specially added P to ar, in below commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 > > > > So it looks like we should keep P here? > > > > But I don't get the point of switching to thin archives. Based on my > > experiment, LTO does not rely on thin archives. The Linux kernel did > > not introduce thin archives for LTO. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f > > So technically it would just be part of dealing with the backlog of > kbuild-resync to take it in this series I guess. > It seems the P flag is needed for ar, otherwise final linking may fail, for example for nokia rx51. Since Linux uses this as well I am just gonna put it there.
diff --git a/Makefile b/Makefile index 6cdd3677eb..33d0b80de8 100644 --- a/Makefile +++ b/Makefile @@ -1750,7 +1750,7 @@ ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(ARCH)/Makefile.postlink) quiet_cmd_u-boot__ ?= LD $@ cmd_u-boot__ ?= $(LD) $(KBUILD_LDFLAGS) $(LDFLAGS_u-boot) -o $@ \ -T u-boot.lds $(u-boot-init) \ - --start-group $(u-boot-main) --end-group \ + --whole-archive $(u-boot-main) --no-whole-archive \ $(PLATFORM_LIBS) -Map u-boot.map; \ $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true) diff --git a/arch/sandbox/config.mk b/arch/sandbox/config.mk index 189e9c2b0c..ebbb094744 100644 --- a/arch/sandbox/config.mk +++ b/arch/sandbox/config.mk @@ -17,13 +17,13 @@ PLATFORM_CPPFLAGS += $(shell $(SDL_CONFIG) --cflags) endif cmd_u-boot__ = $(CC) -o $@ -Wl,-T u-boot.lds $(u-boot-init) \ - -Wl,--start-group $(u-boot-main) -Wl,--end-group \ + -Wl,--whole-archive $(u-boot-main) -Wl,--no-whole-archive \ $(PLATFORM_LIBS) -Wl,-Map -Wl,u-boot.map cmd_u-boot-spl = (cd $(obj) && $(CC) -o $(SPL_BIN) -Wl,-T u-boot-spl.lds \ $(patsubst $(obj)/%,%,$(u-boot-spl-init)) \ - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) -Wl,--no-whole-archive \ + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ $(PLATFORM_LIBS) -Wl,-Map -Wl,u-boot-spl.map -Wl,--gc-sections) CONFIG_ARCH_DEVICE_TREE := sandbox diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 705a886cb9..3659d0af1b 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -331,11 +331,10 @@ $(sort $(subdir-obj-y)): $(subdir-ym) ; # Rule to compile a set of .o files into one .o file # ifdef builtin-target -quiet_cmd_link_o_target = LD $@ +quiet_cmd_link_o_target = AR $@ # If the list of objects to link is empty, just create an empty built-in.o cmd_link_o_target = $(if $(strip $(obj-y)),\ - $(LD) $(ld_flags) -r -o $@ $(filter $(obj-y), $^) \ - $(cmd_secanalysis),\ + rm -f $@; $(AR) cDPrsT $@ $(filter $(obj-y), $^), \ rm -f $@; $(AR) rcs$(KBUILD_ARFLAGS) $@) $(builtin-target): $(obj-y) FORCE diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index ea4e045769..f9faf804de 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -421,10 +421,9 @@ $(obj)/$(SPL_BIN).sym: $(obj)/$(SPL_BIN) FORCE # May be overridden by arch/$(ARCH)/config.mk quiet_cmd_u-boot-spl ?= LD $@ cmd_u-boot-spl ?= (cd $(obj) && $(LD) $(KBUILD_LDFLAGS) $(LDFLAGS_$(@F)) \ - $(patsubst $(obj)/%,%,$(u-boot-spl-init)) --start-group \ - $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) \ - --end-group \ + $(patsubst $(obj)/%,%,$(u-boot-spl-init)) \ + --whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) --no-whole-archive \ + --start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) --end-group \ $(PLATFORM_LIBS) -Map $(SPL_BIN).map -o $(SPL_BIN)) $(obj)/$(SPL_BIN): $(u-boot-spl-platdata) $(u-boot-spl-init) \
Using thin archives instead of incremental linking - saves disk space - works better with dead code elimination - prepares for potential LTO Linux does this for some time now, do this also in U-Boot. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- Makefile | 2 +- arch/sandbox/config.mk | 6 +++--- scripts/Makefile.build | 5 ++--- scripts/Makefile.spl | 7 +++---- 4 files changed, 9 insertions(+), 11 deletions(-)