| Message ID | 1320411209-30476-1-git-send-email-daniel.schwierzeck@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Nov 4, 2011 at 5:53 AM, Daniel Schwierzeck <daniel.schwierzeck@googlemail.com> wrote: > Apply memoization to cc-option macro by caching the results of the > gcc calls. This macro is called very often so using cached results > leads to faster compilation times. > > Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@googlemail.com> Tested-by: Simon Glass <sjg@chromium.org> I see a big speed-up with this: full build 7.05s -> 4.1s incremental 2.25s -> 1.05s > --- > Changes for v2: > - move cache file to $(obj)/include/generated > - reworked completely > - cache also non-working gcc options > - remove CACHE_CC_OPTIONS config switch and enable this optimization > by default > > config.mk | 23 +++++++++++++++++++++-- > 1 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/config.mk b/config.mk > index 918cffe..0da961a 100644 > --- a/config.mk > +++ b/config.mk > @@ -107,8 +107,27 @@ HOSTCFLAGS += -pedantic > # Option checker (courtesy linux kernel) to ensure > # only supported compiler options are used > # > -cc-option = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ > - > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) > +CC_OPTIONS_CACHE_FILE := $(OBJTREE)/include/generated/cc_options.mk > + > +$(if $(wildcard $(CC_OPTIONS_CACHE_FILE)),,\ > + $(shell mkdir -p $(dir $(CC_OPTIONS_CACHE_FILE)))) > + > +sinclude $(CC_OPTIONS_CACHE_FILE) > + > +_ccopt_sys = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ > + > /dev/null 2>&1; then \ > + echo 'CC_OPTIONS += $(strip $1)' >> $(CC_OPTIONS_CACHE_FILE); \ > + echo "$(1)"; else \ > + [ "x$(strip $2)" != "x" ] && \ Do shell still need that x bit? > + echo 'CC_OPTIONS_NOP += $(strip $2)' >> $(CC_OPTIONS_CACHE_FILE); \ > + echo "$(2)"; fi) > + > +_ccopt_cached = $(if $(filter $1,$(CC_OPTIONS)),$1,) Do you need the $(if - doesn't filter give you what you want by itself? > +_ccopt_nop_cached = $(if $(filter $1,$(CC_OPTIONS_NOP)),$1,) > + > +cc-option = $(if $(call _ccopt_cached,$1),$1,\ > + $(if $(call _ccopt_nop_cached,$2),$2,\ > + $(call _ccopt_sys,$1,$2))) > > # > # Include the make variables (CC, etc...) > -- > 1.7.7.1 > > Thanks for doing this! Regards, Simon
Hi SImon, On Fri, Nov 4, 2011 at 5:32 PM, Simon Glass <sjg@chromium.org> wrote: > On Fri, Nov 4, 2011 at 5:53 AM, Daniel Schwierzeck > <daniel.schwierzeck@googlemail.com> wrote: >> Apply memoization to cc-option macro by caching the results of the >> gcc calls. This macro is called very often so using cached results >> leads to faster compilation times. >> >> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@googlemail.com> > > Tested-by: Simon Glass <sjg@chromium.org> > > I see a big speed-up with this: > > full build 7.05s -> 4.1s > incremental 2.25s -> 1.05s > >> --- >> Changes for v2: >> - move cache file to $(obj)/include/generated >> - reworked completely >> - cache also non-working gcc options >> - remove CACHE_CC_OPTIONS config switch and enable this optimization >> by default >> >> config.mk | 23 +++++++++++++++++++++-- >> 1 files changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/config.mk b/config.mk >> index 918cffe..0da961a 100644 >> --- a/config.mk >> +++ b/config.mk >> @@ -107,8 +107,27 @@ HOSTCFLAGS += -pedantic >> # Option checker (courtesy linux kernel) to ensure >> # only supported compiler options are used >> # >> -cc-option = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ >> - > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) >> +CC_OPTIONS_CACHE_FILE := $(OBJTREE)/include/generated/cc_options.mk >> + >> +$(if $(wildcard $(CC_OPTIONS_CACHE_FILE)),,\ >> + $(shell mkdir -p $(dir $(CC_OPTIONS_CACHE_FILE)))) >> + >> +sinclude $(CC_OPTIONS_CACHE_FILE) >> + >> +_ccopt_sys = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ >> + > /dev/null 2>&1; then \ >> + echo 'CC_OPTIONS += $(strip $1)' >> $(CC_OPTIONS_CACHE_FILE); \ >> + echo "$(1)"; else \ >> + [ "x$(strip $2)" != "x" ] && \ > > Do shell still need that x bit? Probably not but it is safer and does not really harm ;) > >> + echo 'CC_OPTIONS_NOP += $(strip $2)' >> $(CC_OPTIONS_CACHE_FILE); \ >> + echo "$(2)"; fi) >> + >> +_ccopt_cached = $(if $(filter $1,$(CC_OPTIONS)),$1,) > > Do you need the $(if - doesn't filter give you what you want by itself? You are right, the if is redundant. I will optimize this. > >> +_ccopt_nop_cached = $(if $(filter $1,$(CC_OPTIONS_NOP)),$1,) >> + >> +cc-option = $(if $(call _ccopt_cached,$1),$1,\ >> + $(if $(call _ccopt_nop_cached,$2),$2,\ >> + $(call _ccopt_sys,$1,$2))) >> >> # >> # Include the make variables (CC, etc...) >> -- >> 1.7.7.1 >> >> > > Thanks for doing this! > > Regards, > Simon > Thanks, Daniel
Hi Albert, Wolfgang, following code from arch/arm/config does not work correctly with my optimization PF_CPPFLAGS_ABI := $(call cc-option,\ -mabi=aapcs-linux -mno-thumb-interwork,\ $(call cc-option,\ -mapcs-32,\ $(call cc-option,\ -mabi=apcs-gnu,\ )\ ) $(call cc-option,-mno-thumb-interwork,)\ ) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARM) $(PF_CPPFLAGS_ABI) Compiling with seabord_config and latest CodeSourcery toolchain the generated cache file has following content: CC_OPTIONS += -marm CC_OPTIONS += -mabi=apcs-gnu CC_OPTIONS_NOP += -mabi=apcs-gnu CC_OPTIONS += -mno-thumb-interwork CC_OPTIONS += -mabi=aapcs-linux -mno-thumb-interwork CC_OPTIONS += -fno-stack-protector CC_OPTIONS += -Wno-format-nonliteral CC_OPTIONS += -Wno-format-security CC_OPTIONS += -fno-toplevel-reorder If I rewrite that code to PF_CPPFLAGS_ABI := $(call cc-option,-mabi=aapcs-linux,) ifeq ($(PF_CPPFLAGS_ABI),) PF_CPPFLAGS_ABI := $(call cc-option,-mapcs-32,-mabi=apcs-gnu) endif PF_CPPFLAGS_THUMB := $(call cc-option,-mno-thumb-interwork,) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARM) $(PF_CPPFLAGS_ABI) $(PF_CPPFLAGS_THUMB) the cache file looks better: CC_OPTIONS += -marm CC_OPTIONS += -mabi=aapcs-linux CC_OPTIONS += -mno-thumb-interwork CC_OPTIONS += -fno-stack-protector CC_OPTIONS += -Wno-format-nonliteral CC_OPTIONS += -Wno-format-security CC_OPTIONS += -fno-toplevel-reorder Should we change it? is the semantic still the same? Best regards, Daniel
Dear Daniel Schwierzeck, In message <CACUy__XY1873+dT-s=-GyPeBq5hnYsmYwWt-gxEXiwY97EcGdg@mail.gmail.com> you wrote: > > Should we change it? is the semantic still the same? I'm not sure. At first reading it doesn't look really the same to me. Best regards, Wolfgang Denk
Hi all, Le 04/11/2011 18:56, Wolfgang Denk a écrit : > Dear Daniel Schwierzeck, > > In message<CACUy__XY1873+dT-s=-GyPeBq5hnYsmYwWt-gxEXiwY97EcGdg@mail.gmail.com> you wrote: >> >> Should we change it? is the semantic still the same? > > I'm not sure. At first reading it doesn't look really the same to me. They are not, at least for ELDK4.2. The only difference is in -mabi options, where the change would reduce "-mabi=apcs-gnu -mabi=aapcs-linux" to "-mabi=aapcs-linux". apcs-gnu, IIUC, is 'old ABI', while 'aapcs-linux' is 'new ABI', aka eabi. Most of the toolchains I see are eabi (ELDK and CS notably). There may be 'old ABI' toolchains out there, but I don't think they are old ABI either. Anyway, I've just tried ./MAKEALL edminiv2 with ELD42 and a couple of CS toolchains, and nowhere in the log does -mabi=apcs-gnu show up -- the gcc invocations only have -mabi=aapcs-linux. I've also tested making ED Mini V2 with and without the patch but without Daniel's proposed change to arch/arm/config.mk, and there is no difference in build commands (except that for some reason the patch inserts multiple spaces between some gcc invocation options. Daniel, what do you mean with "does not work correctly"? > Best regards, > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4EB4FF09.5070601@aribaud.net> you wrote: > > apcs-gnu, IIUC, is 'old ABI', while 'aapcs-linux' is 'new ABI', aka > eabi. Most of the toolchains I see are eabi (ELDK and CS notably). There > may be 'old ABI' toolchains out there, but I don't think they are old > ABI either. > > Anyway, I've just tried ./MAKEALL edminiv2 with ELD42 and a couple of CS > toolchains, and nowhere in the log does -mabi=apcs-gnu show up -- the > gcc invocations only have -mabi=aapcs-linux. Try ELDK 4.1 ... Best regards, Wolfgang Denk
Hi Albert, On 05.11.2011 10:16, Albert ARIBAUD wrote: > Hi all, > > Le 04/11/2011 18:56, Wolfgang Denk a écrit : >> Dear Daniel Schwierzeck, >> >> In >> message<CACUy__XY1873+dT-s=-GyPeBq5hnYsmYwWt-gxEXiwY97EcGdg@mail.gmail.com> >> you wrote: >>> >>> Should we change it? is the semantic still the same? >> >> I'm not sure. At first reading it doesn't look really the same to me. > > They are not, at least for ELDK4.2. > > The only difference is in -mabi options, where the change would reduce > "-mabi=apcs-gnu -mabi=aapcs-linux" to "-mabi=aapcs-linux". > > apcs-gnu, IIUC, is 'old ABI', while 'aapcs-linux' is 'new ABI', aka > eabi. Most of the toolchains I see are eabi (ELDK and CS notably). There > may be 'old ABI' toolchains out there, but I don't think they are old > ABI either. > > Anyway, I've just tried ./MAKEALL edminiv2 with ELD42 and a couple of CS > toolchains, and nowhere in the log does -mabi=apcs-gnu show up -- the > gcc invocations only have -mabi=aapcs-linux. Looks like I read it wrong. So you always want "-mabi=apcs-gnu -mabi=aapcs-linux -mno-thumb-interwork" in $(PF_CPPFLAGS_ABI) with EABI? Sorry but I am not an ARM expert ;) > > I've also tested making ED Mini V2 with and without the patch but > without Daniel's proposed change to arch/arm/config.mk, and there is no > difference in build commands (except that for some reason the patch > inserts multiple spaces between some gcc invocation options. I guess this comes from making the macros more readable. Maybe I can optimize this. > > Daniel, what do you mean with "does not work correctly"? that the generated cache file looks not right CC_OPTIONS += -mabi=apcs-gnu CC_OPTIONS_NOP += -mabi=apcs-gnu CC_OPTIONS += -mno-thumb-interwork CC_OPTIONS += -mabi=aapcs-linux -mno-thumb-interwork But if you want "-mabi=apcs-gnu -mabi=aapcs-linux -mno-thumb-interwork" then it should already work correctly without my change in arch/arm/config.mk. Best regards, Daniel
Hi Daniel, Le 05/11/2011 14:43, Daniel Schwierzeck a écrit : > Hi Albert, > > On 05.11.2011 10:16, Albert ARIBAUD wrote: >> Hi all, >> >> Le 04/11/2011 18:56, Wolfgang Denk a écrit : >>> Dear Daniel Schwierzeck, >>> >>> In >>> message<CACUy__XY1873+dT-s=-GyPeBq5hnYsmYwWt-gxEXiwY97EcGdg@mail.gmail.com> >>> >>> you wrote: >>>> >>>> Should we change it? is the semantic still the same? >>> >>> I'm not sure. At first reading it doesn't look really the same to me. >> >> They are not, at least for ELDK4.2. >> >> The only difference is in -mabi options, where the change would reduce >> "-mabi=apcs-gnu -mabi=aapcs-linux" to "-mabi=aapcs-linux". >> >> apcs-gnu, IIUC, is 'old ABI', while 'aapcs-linux' is 'new ABI', aka >> eabi. Most of the toolchains I see are eabi (ELDK and CS notably). There >> may be 'old ABI' toolchains out there, but I don't think they are old >> ABI either. >> >> Anyway, I've just tried ./MAKEALL edminiv2 with ELD42 and a couple of CS >> toolchains, and nowhere in the log does -mabi=apcs-gnu show up -- the >> gcc invocations only have -mabi=aapcs-linux. > > Looks like I read it wrong. So you always want "-mabi=apcs-gnu > -mabi=aapcs-linux -mno-thumb-interwork" in $(PF_CPPFLAGS_ABI) with EABI? > Sorry but I am not an ARM expert ;) No, I don't want that. :) The problem I see is having two conflicting -mabi options, -mabi=apcs-gnu and -mabi=aapcs-linux, in the same command line. There should be only one -- and it should be the same across the whole U-Boot building process. The duplicate -mno-thumb-interwork does not worry me fronm a functional standpoint; it's just a waste of space, that's all. >> I've also tested making ED Mini V2 with and without the patch but >> without Daniel's proposed change to arch/arm/config.mk, and there is no >> difference in build commands (except that for some reason the patch >> inserts multiple spaces between some gcc invocation options. > > I guess this comes from making the macros more readable. Maybe I can > optimize this. > >> >> Daniel, what do you mean with "does not work correctly"? > > that the generated cache file looks not right > > CC_OPTIONS += -mabi=apcs-gnu > CC_OPTIONS_NOP += -mabi=apcs-gnu > CC_OPTIONS += -mno-thumb-interwork > CC_OPTIONS += -mabi=aapcs-linux -mno-thumb-interwork I'm not a makefile expert, so "looks not right" is a bit meaningless to me. If "not right" means "there are several conflicting -mabi options and there are repeated -mno-thumb-interwork options" then I agree with your "does not look right" statement. > But if you want "-mabi=apcs-gnu -mabi=aapcs-linux -mno-thumb-interwork" > then it should already work correctly without my change in > arch/arm/config.mk. As I said, no, I don't want that. I want the right -mabi option only, and I'd like a single -mno-thumb-interwork option. But what I don't understand is the discrepancy which I see between the CC_OPTIONS resulting from include/generated/cc_options.mk and the actual command line options used to generate e.g. edminiv2. > Best regards, > Daniel Amicalement,
diff --git a/config.mk b/config.mk index 918cffe..0da961a 100644 --- a/config.mk +++ b/config.mk @@ -107,8 +107,27 @@ HOSTCFLAGS += -pedantic # Option checker (courtesy linux kernel) to ensure # only supported compiler options are used # -cc-option = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ - > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) +CC_OPTIONS_CACHE_FILE := $(OBJTREE)/include/generated/cc_options.mk + +$(if $(wildcard $(CC_OPTIONS_CACHE_FILE)),,\ + $(shell mkdir -p $(dir $(CC_OPTIONS_CACHE_FILE)))) + +sinclude $(CC_OPTIONS_CACHE_FILE) + +_ccopt_sys = $(shell if $(CC) $(CFLAGS) $(1) -S -o /dev/null -xc /dev/null \ + > /dev/null 2>&1; then \ + echo 'CC_OPTIONS += $(strip $1)' >> $(CC_OPTIONS_CACHE_FILE); \ + echo "$(1)"; else \ + [ "x$(strip $2)" != "x" ] && \ + echo 'CC_OPTIONS_NOP += $(strip $2)' >> $(CC_OPTIONS_CACHE_FILE); \ + echo "$(2)"; fi) + +_ccopt_cached = $(if $(filter $1,$(CC_OPTIONS)),$1,) +_ccopt_nop_cached = $(if $(filter $1,$(CC_OPTIONS_NOP)),$1,) + +cc-option = $(if $(call _ccopt_cached,$1),$1,\ + $(if $(call _ccopt_nop_cached,$2),$2,\ + $(call _ccopt_sys,$1,$2))) # # Include the make variables (CC, etc...)
Apply memoization to cc-option macro by caching the results of the gcc calls. This macro is called very often so using cached results leads to faster compilation times. Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@googlemail.com> --- Changes for v2: - move cache file to $(obj)/include/generated - reworked completely - cache also non-working gcc options - remove CACHE_CC_OPTIONS config switch and enable this optimization by default config.mk | 23 +++++++++++++++++++++-- 1 files changed, 21 insertions(+), 2 deletions(-)