2015-06-18 06:55:50

by Vineet Gupta

[permalink] [raw]
Subject: subtle side effect of commit a1c48bb160f836

Hi Geert,

commit a1c48bb160f8368 "Makefile: Fix unrecognized cross-compiler command line
options" moved ARCH specific cc option handling before common -Os/O2 setup.

For ARC this had a subtle effect that we can no longer over-ride generic -O2 with
-O3, hence a performance regression observed going from 3.13 to 3.18 (the above
commit went into 3.16)

I want to understand how to properly fix this. Moving the include of arch makefile
will bring back the old issue. I can introduce another option to set default optim
level, but only arc/m32r care about it anyways.

Thx,
-Vineet


2015-06-18 07:10:38

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

Hi Vineet,

On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
<[email protected]> wrote:
> commit a1c48bb160f8368 "Makefile: Fix unrecognized cross-compiler command line
> options" moved ARCH specific cc option handling before common -Os/O2 setup.
>
> For ARC this had a subtle effect that we can no longer over-ride generic -O2 with
> -O3, hence a performance regression observed going from 3.13 to 3.18 (the above
> commit went into 3.16)
>
> I want to understand how to properly fix this. Moving the include of arch makefile
> will bring back the old issue. I can introduce another option to set default optim
> level, but only arc/m32r care about it anyways.

Can we include $(srctree)/arch/$(SRCARCH)/Makefile twice?

Or perhaps we can not apply the extra -O* if there's already a -O* option?

Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
a(nother) Kconfig option may make sense.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2015-06-18 07:33:39

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thursday 18 June 2015 12:40 PM, Geert Uytterhoeven wrote:
> On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
> <[email protected]> wrote:
>> > commit a1c48bb160f8368 "Makefile: Fix unrecognized cross-compiler command line
>> > options" moved ARCH specific cc option handling before common -Os/O2 setup.
>> >
>> > For ARC this had a subtle effect that we can no longer over-ride generic -O2 with
>> > -O3, hence a performance regression observed going from 3.13 to 3.18 (the above
>> > commit went into 3.16)
>> >
>> > I want to understand how to properly fix this. Moving the include of arch makefile
>> > will bring back the old issue. I can introduce another option to set default optim
>> > level, but only arc/m32r care about it anyways.
> Can we include $(srctree)/arch/$(SRCARCH)/Makefile twice?

Something like this would be ideal, but does that not bring back your warnings ?

>
> Or perhaps we can not apply the extra -O* if there's already a -O* option?

Could be, but I'm not sure how to do that ?


> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
> a(nother) Kconfig option may make sense.

I can cook this one - but is it really worth doing when only 2 arches care.

Michal, do you have any opinion on how to solve this ?

-Vineet

2015-06-18 07:37:21

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thu, Jun 18, 2015 at 9:33 AM, Vineet Gupta
<[email protected]> wrote:
> On Thursday 18 June 2015 12:40 PM, Geert Uytterhoeven wrote:
>> On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
>> <[email protected]> wrote:
>>> > commit a1c48bb160f8368 "Makefile: Fix unrecognized cross-compiler command line
>>> > options" moved ARCH specific cc option handling before common -Os/O2 setup.
>>> >
>>> > For ARC this had a subtle effect that we can no longer over-ride generic -O2 with
>>> > -O3, hence a performance regression observed going from 3.13 to 3.18 (the above
>>> > commit went into 3.16)
>>> >
>>> > I want to understand how to properly fix this. Moving the include of arch makefile
>>> > will bring back the old issue. I can introduce another option to set default optim
>>> > level, but only arc/m32r care about it anyways.
>> Can we include $(srctree)/arch/$(SRCARCH)/Makefile twice?
>
> Something like this would be ideal, but does that not bring back your warnings ?

I don't think so. The warnings were caused by using the host compiler instead
of the cross compiler while checking for the support of some compiler options.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2015-06-18 08:00:45

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thursday 18 June 2015 01:07 PM, Geert Uytterhoeven wrote:
>>> >> Can we include $(srctree)/arch/$(SRCARCH)/Makefile twice?
>> >
>> > Something like this would be ideal, but does that not bring back your warnings ?
> I don't think so. The warnings were caused by using the host compiler instead
> of the cross compiler while checking for the support of some compiler options.

Tried that but build system spews tons of warning about over-riding commands...

----->8-------
arch/arc/Makefile:103: warning: overriding commands for target `uImage'
arch/arc/Makefile:103: warning: ignoring old commands for target `uImage'
arch/arc/Makefile:103: warning: overriding commands for target `uImage.bin'
arch/arc/Makefile:103: warning: ignoring old commands for target `uImage.bin'
...
...
----->8-------


2015-06-18 08:15:33

by Michal Marek

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thu, Jun 18, 2015 at 09:10:30AM +0200, Geert Uytterhoeven wrote:
> Hi Vineet,
>
> On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
> <[email protected]> wrote:
> > commit a1c48bb160f8368 "Makefile: Fix unrecognized cross-compiler command line
> > options" moved ARCH specific cc option handling before common -Os/O2 setup.
> >
> > For ARC this had a subtle effect that we can no longer over-ride generic -O2 with
> > -O3, hence a performance regression observed going from 3.13 to 3.18 (the above
> > commit went into 3.16)
> >
> > I want to understand how to properly fix this. Moving the include of arch makefile
> > will bring back the old issue. I can introduce another option to set default optim
> > level, but only arc/m32r care about it anyways.

m32r only sets -O for its assembler.


> Can we include $(srctree)/arch/$(SRCARCH)/Makefile twice?

Please don't, the gcc commandline is long enough already.


> Or perhaps we can not apply the extra -O* if there's already a -O* option?
>
> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
> a(nother) Kconfig option may make sense.

We can also introduce some ARCH_CFLAGS that is appended near the end of
the list, and have arc/Makefile add its -O3 there. But I'd like to why
the -O3 needs to be there in first place. Obviously, the kernel works
with -O2, otherwise the regression would have been identified earlier.
So why can't users specify -O3 in KCFLAGS like on any other
architecture.

Michal

2015-06-18 08:46:11

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>> > Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>> > a(nother) Kconfig option may make sense.
> We can also introduce some ARCH_CFLAGS that is appended near the end of
> the list, and have arc/Makefile add its -O3 there. But I'd like to why
> the -O3 needs to be there in first place.

This is how historically ARC kernels have been built. We do track performance
results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
of the numbers when we measured 3.18 (vs. 3.13)

> Obviously, the kernel works
> with -O2, otherwise the regression would have been identified earlier.

Its a performance thing - so yeah -O2 works, but -O3 works even better :-)

> So why can't users specify -O3 in KCFLAGS like on any other
> architecture.

Sweet, I didn't know about this. But I don't see any arch setting this - only tile
using it. So yeah - below does the trick !

--------->
>From 5e44cd2ed69b1d030b4cb87600d2767b69c35537 Mon Sep 17 00:00:00 2001
From: Vineet Gupta <[email protected]>
Date: Thu, 18 Jun 2015 13:54:01 +0530
Subject: [PATCH] ARC: Override toplevel default -O2 with -O3

ARC kernels have historically been built with -O3, despite top level
Makefile defaulting to -O2. This was facilitated by implicitly ordering
of arch makefile include AFTER top level assigned -O2.

An upstream fix to top level a1c48bb160f ("Makefile: Fix unrecognized
cross-compiler command line options") changed the ordering, making ARC
-O3 defunt.

Fix that by NOT relying on any ordering whatsoever and use the right
mechanism to do the over-rides.

Suggested-by: Michal Marek <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/Makefile | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arc/Makefile b/arch/arc/Makefile
index 86c71b2089d2..c23f3f2b0ff8 100644
--- a/arch/arc/Makefile
+++ b/arch/arc/Makefile
@@ -44,6 +44,7 @@ endif
ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
# Generic build system uses -O2, we want -O3
cflags-y += -O3
+KCFLAGS += -O3
endif

# small data is default for elf32 tool-chain. If not usable, disable it
--
1.9.1

2015-06-18 08:55:29

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

Hi Vineet,

On Thu, Jun 18, 2015 at 10:45 AM, Vineet Gupta
<[email protected]> wrote:
> On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>>> > Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>>> > a(nother) Kconfig option may make sense.
>> We can also introduce some ARCH_CFLAGS that is appended near the end of
>> the list, and have arc/Makefile add its -O3 there. But I'd like to why
>> the -O3 needs to be there in first place.
>
> This is how historically ARC kernels have been built. We do track performance
> results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
> of the numbers when we measured 3.18 (vs. 3.13)
>
>> Obviously, the kernel works
>> with -O2, otherwise the regression would have been identified earlier.
>
> Its a performance thing - so yeah -O2 works, but -O3 works even better :-)

Did you see some numbers increase when going from -O3 to -O2?
IIRC, -O3 enables more aggressive inlining, which can cause more L1 cache
misses.

It might be worth trying CONFIG_CC_OPTIMIZE_FOR_SIZE=y...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2015-06-18 09:17:23

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

+CC Claudiu - ARC gcc guru

On Thursday 18 June 2015 02:25 PM, Geert Uytterhoeven wrote:
> Hi Vineet,
>
> On Thu, Jun 18, 2015 at 10:45 AM, Vineet Gupta
> <[email protected]> wrote:
>> On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>>>>> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>>>>> a(nother) Kconfig option may make sense.
>>> We can also introduce some ARCH_CFLAGS that is appended near the end of
>>> the list, and have arc/Makefile add its -O3 there. But I'd like to why
>>> the -O3 needs to be there in first place.
>>
>> This is how historically ARC kernels have been built. We do track performance
>> results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
>> of the numbers when we measured 3.18 (vs. 3.13)
>>
>>> Obviously, the kernel works
>>> with -O2, otherwise the regression would have been identified earlier.
>>
>> Its a performance thing - so yeah -O2 works, but -O3 works even better :-)
>
> Did you see some numbers increase when going from -O3 to -O2?
> IIRC, -O3 enables more aggressive inlining, which can cause more L1 cache
> misses.

It sure does but smaller functions could cause more stack return mispredicts etc.
It all boils down to the micro-arch in the end and how gcc does arch specific
things under the hood of -O{2,s}.


> It might be worth trying CONFIG_CC_OPTIMIZE_FOR_SIZE=y...

Not for ARC. At -Os gcc is more worried about using short instructions (2 bytes),
and things like alignment of target branches, ld/st scheduling might not be as
optim as with -O2/O3. Some of the code density instructions have associated
pipeline stalls etc.

So last time (it's been a while though) when I ran benchmarks with -Os on ARC, it
was way off vs. -O2.

2015-06-18 10:14:45

by Michal Marek

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

Dne 18.6.2015 v 10:45 Vineet Gupta napsal(a):
> On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>>>> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>>>> a(nother) Kconfig option may make sense.
>> We can also introduce some ARCH_CFLAGS that is appended near the end of
>> the list, and have arc/Makefile add its -O3 there. But I'd like to why
>> the -O3 needs to be there in first place.
>
> This is how historically ARC kernels have been built. We do track performance
> results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
> of the numbers when we measured 3.18 (vs. 3.13)
>
>> Obviously, the kernel works
>> with -O2, otherwise the regression would have been identified earlier.
>
> Its a performance thing - so yeah -O2 works, but -O3 works even better :-)
>
>> So why can't users specify -O3 in KCFLAGS like on any other
>> architecture.
>
> Sweet, I didn't know about this. But I don't see any arch setting this - only tile
> using it. So yeah - below does the trick !
>
> --------->
> From 5e44cd2ed69b1d030b4cb87600d2767b69c35537 Mon Sep 17 00:00:00 2001
> From: Vineet Gupta <[email protected]>
> Date: Thu, 18 Jun 2015 13:54:01 +0530
> Subject: [PATCH] ARC: Override toplevel default -O2 with -O3
>
> ARC kernels have historically been built with -O3, despite top level
> Makefile defaulting to -O2. This was facilitated by implicitly ordering
> of arch makefile include AFTER top level assigned -O2.
>
> An upstream fix to top level a1c48bb160f ("Makefile: Fix unrecognized
> cross-compiler command line options") changed the ordering, making ARC
> -O3 defunt.
>
> Fix that by NOT relying on any ordering whatsoever and use the right
> mechanism to do the over-rides.
>
> Suggested-by: Michal Marek <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Vineet Gupta <[email protected]>
> ---
> arch/arc/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arc/Makefile b/arch/arc/Makefile
> index 86c71b2089d2..c23f3f2b0ff8 100644
> --- a/arch/arc/Makefile
> +++ b/arch/arc/Makefile
> @@ -44,6 +44,7 @@ endif
> ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
> # Generic build system uses -O2, we want -O3
> cflags-y += -O3
> +KCFLAGS += -O3
> endif

Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
be set in the environment or command line.

Michal

2015-06-18 10:33:14

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Thursday 18 June 2015 03:44 PM, Michal Marek wrote:
> Dne 18.6.2015 v 10:45 Vineet Gupta napsal(a):
>> > On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>>>>> >>>> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>>>>> >>>> a(nother) Kconfig option may make sense.
>>> >> We can also introduce some ARCH_CFLAGS that is appended near the end of
>>> >> the list, and have arc/Makefile add its -O3 there. But I'd like to why
>>> >> the -O3 needs to be there in first place.
>> >
>> > This is how historically ARC kernels have been built. We do track performance
>> > results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
>> > of the numbers when we measured 3.18 (vs. 3.13)
>> >
>>> >> Obviously, the kernel works
>>> >> with -O2, otherwise the regression would have been identified earlier.
>> >
>> > Its a performance thing - so yeah -O2 works, but -O3 works even better :-)
>> >
>>> >> So why can't users specify -O3 in KCFLAGS like on any other
>>> >> architecture.
>> >
>> > Sweet, I didn't know about this. But I don't see any arch setting this - only tile
>> > using it. So yeah - below does the trick !
>> >
>> > --------->
>> > From 5e44cd2ed69b1d030b4cb87600d2767b69c35537 Mon Sep 17 00:00:00 2001
>> > From: Vineet Gupta <[email protected]>
>> > Date: Thu, 18 Jun 2015 13:54:01 +0530
>> > Subject: [PATCH] ARC: Override toplevel default -O2 with -O3
>> >
>> > ARC kernels have historically been built with -O3, despite top level
>> > Makefile defaulting to -O2. This was facilitated by implicitly ordering
>> > of arch makefile include AFTER top level assigned -O2.
>> >
>> > An upstream fix to top level a1c48bb160f ("Makefile: Fix unrecognized
>> > cross-compiler command line options") changed the ordering, making ARC
>> > -O3 defunt.
>> >
>> > Fix that by NOT relying on any ordering whatsoever and use the right
>> > mechanism to do the over-rides.
>> >
>> > Suggested-by: Michal Marek <[email protected]>
>> > Cc: Geert Uytterhoeven <[email protected]>
>> > Cc: <[email protected]>
>> > Signed-off-by: Vineet Gupta <[email protected]>
>> > ---
>> > arch/arc/Makefile | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/arch/arc/Makefile b/arch/arc/Makefile
>> > index 86c71b2089d2..c23f3f2b0ff8 100644
>> > --- a/arch/arc/Makefile
>> > +++ b/arch/arc/Makefile
>> > @@ -44,6 +44,7 @@ endif
>> > ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
>> > # Generic build system uses -O2, we want -O3
>> > cflags-y += -O3
>> > +KCFLAGS += -O3
>> > endif
> Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
> be set in the environment or command line.

Well I don't want to rely on external settings whatsoever to enforce this. If this
is not the right way, what do u suggest I do to help fix this.

-Vineet

2015-06-24 12:21:02

by Vineet Gupta

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

Hi Michal,

On Thursday 18 June 2015 04:02 PM, Vineet Gupta wrote:
> On Thursday 18 June 2015 03:44 PM, Michal Marek wrote:
>> Dne 18.6.2015 v 10:45 Vineet Gupta napsal(a):
>>>> On Thursday 18 June 2015 01:43 PM, Michal Marek wrote:
>>>>>>>>>> Alternatively, as we already have CONFIG_CC_OPTIMIZE_FOR_SIZE,
>>>>>>>>>> a(nother) Kconfig option may make sense.
>>>>>> We can also introduce some ARCH_CFLAGS that is appended near the end of
>>>>>> the list, and have arc/Makefile add its -O3 there. But I'd like to why
>>>>>> the -O3 needs to be there in first place.
>>>>
>>>> This is how historically ARC kernels have been built. We do track performance
>>>> results LMBench/hackbench... and going from -O3 to -O2 caused a sudden dip in some
>>>> of the numbers when we measured 3.18 (vs. 3.13)
>>>>
>>>>>> Obviously, the kernel works
>>>>>> with -O2, otherwise the regression would have been identified earlier.
>>>>
>>>> Its a performance thing - so yeah -O2 works, but -O3 works even better :-)
>>>>
>>>>>> So why can't users specify -O3 in KCFLAGS like on any other
>>>>>> architecture.
>>>>
>>>> Sweet, I didn't know about this. But I don't see any arch setting this - only tile
>>>> using it. So yeah - below does the trick !
>>>>
>>>> --------->
>>>> From 5e44cd2ed69b1d030b4cb87600d2767b69c35537 Mon Sep 17 00:00:00 2001
>>>> From: Vineet Gupta <[email protected]>
>>>> Date: Thu, 18 Jun 2015 13:54:01 +0530
>>>> Subject: [PATCH] ARC: Override toplevel default -O2 with -O3
>>>>
>>>> ARC kernels have historically been built with -O3, despite top level
>>>> Makefile defaulting to -O2. This was facilitated by implicitly ordering
>>>> of arch makefile include AFTER top level assigned -O2.
>>>>
>>>> An upstream fix to top level a1c48bb160f ("Makefile: Fix unrecognized
>>>> cross-compiler command line options") changed the ordering, making ARC
>>>> -O3 defunt.
>>>>
>>>> Fix that by NOT relying on any ordering whatsoever and use the right
>>>> mechanism to do the over-rides.
>>>>
>>>> Suggested-by: Michal Marek <[email protected]>
>>>> Cc: Geert Uytterhoeven <[email protected]>
>>>> Cc: <[email protected]>
>>>> Signed-off-by: Vineet Gupta <[email protected]>
>>>> ---
>>>> arch/arc/Makefile | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arc/Makefile b/arch/arc/Makefile
>>>> index 86c71b2089d2..c23f3f2b0ff8 100644
>>>> --- a/arch/arc/Makefile
>>>> +++ b/arch/arc/Makefile
>>>> @@ -44,6 +44,7 @@ endif
>>>> ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
>>>> # Generic build system uses -O2, we want -O3
>>>> cflags-y += -O3
>>>> +KCFLAGS += -O3
>>>> endif
>> Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
>> be set in the environment or command line.
>
> Well I don't want to rely on external settings whatsoever to enforce this. If this
> is not the right way, what do u suggest I do to help fix this.


Can I keep this seeming abuse of KCFLAGS or do u suggest alternate approach I can
code up !

Thx,
-Vineet

2015-07-01 15:19:39

by Michal Marek

[permalink] [raw]
Subject: Re: subtle side effect of commit a1c48bb160f836

On Wed, Jun 24, 2015 at 05:50:16PM +0530, Vineet Gupta wrote:
> On Thursday 18 June 2015 04:02 PM, Vineet Gupta wrote:
> > On Thursday 18 June 2015 03:44 PM, Michal Marek wrote:
> >> Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
> >> be set in the environment or command line.
> >
> > Well I don't want to rely on external settings whatsoever to enforce this. If this
> > is not the right way, what do u suggest I do to help fix this.
>
>
> Can I keep this seeming abuse of KCFLAGS or do u suggest alternate approach I can
> code up !

Hi Vineet, sorry for the late reply. I had something like the below
patch in mind, simply allow arc to specify -O3 in ARCH_CFLAGS (that part
I'm leaving up to you).


>From e458fdf4ae37e1adce81b58d96b1075b4f0656e6 Mon Sep 17 00:00:00 2001
From: Michal Marek <[email protected]>
Date: Wed, 1 Jul 2015 17:13:16 +0200
Subject: [PATCH] kbuild: Allow arch Makefiles to override {cpp,ld,c}flags

Since commit a1c48bb1 (Makefile: Fix unrecognized cross-compiler command
line options), the arch Makefile is included earlier by the main
Makefile, preventing the arc architecture to set its -O3 compiler
option. Since there might be more use cases for an arch Makefile to
fine-tune the options, add support for ARCH_CPPFLAGS, ARCH_AFLAGS and
ARCH_CFLAGS variables that are appended to the respective kbuild
variables. The user still has the final say via the KCPPFLAGS, KAFLAGS
and KCFLAGS variables.

Reported-by: Vineet Gupta <[email protected]>
Signed-off-by: Michal Marek <[email protected]>
---
Documentation/kbuild/makefiles.txt | 8 ++++++++
Makefile | 9 +++++----
2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 74b6c6d..d2b1c40 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -952,6 +952,14 @@ When kbuild executes, the following steps are followed (roughly):
$(KBUILD_ARFLAGS) set by the top level Makefile to "D" (deterministic
mode) if this option is supported by $(AR).

+ ARCH_CPPFLAGS, ARCH_AFLAGS, ARCH_CFLAGS Overrides the kbuild defaults
+
+ These variables are appended to the KBUILD_CPPFLAGS,
+ KBUILD_AFLAGS, and KBUILD_CFLAGS, respectively, after the
+ top-level Makefile has set any other flags. This provides a
+ means for an architecture to override the defaults.
+
+
--- 6.2 Add prerequisites to archheaders:

The archheaders: rule is used to generate header files that
diff --git a/Makefile b/Makefile
index 3ba5044..aa0dfbe 100644
--- a/Makefile
+++ b/Makefile
@@ -783,10 +783,11 @@ endif
include scripts/Makefile.kasan
include scripts/Makefile.extrawarn

-# Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
-KBUILD_CPPFLAGS += $(KCPPFLAGS)
-KBUILD_AFLAGS += $(KAFLAGS)
-KBUILD_CFLAGS += $(KCFLAGS)
+# Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
+# last assignments
+KBUILD_CPPFLAGS += $(ARCH_CPPFLAGS) $(KCPPFLAGS)
+KBUILD_AFLAGS += $(ARCH_AFLAGS) $(KAFLAGS)
+KBUILD_CFLAGS += $(ARCH_CFLAGS) $(KCFLAGS)

# Use --build-id when available.
LDFLAGS_BUILD_ID = $(patsubst -Wl$(comma)%,%,\
--
2.1.4

2015-07-02 05:27:53

by Vineet Gupta

[permalink] [raw]
Subject: ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

On Wednesday 01 July 2015 08:49 PM, Michal Marek wrote:
> On Wed, Jun 24, 2015 at 05:50:16PM +0530, Vineet Gupta wrote:
>> On Thursday 18 June 2015 04:02 PM, Vineet Gupta wrote:
>>> On Thursday 18 June 2015 03:44 PM, Michal Marek wrote:
>>>> Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
>>>> be set in the environment or command line.
>>>
>>> Well I don't want to rely on external settings whatsoever to enforce this. If this
>>> is not the right way, what do u suggest I do to help fix this.
>>
>>
>> Can I keep this seeming abuse of KCFLAGS or do u suggest alternate approach I can
>> code up !
>
> Hi Vineet, sorry for the late reply.

NP !

> I had something like the below
> patch in mind, simply allow arc to specify -O3 in ARCH_CFLAGS (that part
> I'm leaving up to you).

See below !

> From e458fdf4ae37e1adce81b58d96b1075b4f0656e6 Mon Sep 17 00:00:00 2001
> From: Michal Marek <[email protected]>
> Date: Wed, 1 Jul 2015 17:13:16 +0200
> Subject: [PATCH] kbuild: Allow arch Makefiles to override {cpp,ld,c}flags
>
> Since commit a1c48bb1 (Makefile: Fix unrecognized cross-compiler command
> line options), the arch Makefile is included earlier by the main
> Makefile, preventing the arc architecture to set its -O3 compiler
> option. Since there might be more use cases for an arch Makefile to
> fine-tune the options, add support for ARCH_CPPFLAGS, ARCH_AFLAGS and
> ARCH_CFLAGS variables that are appended to the respective kbuild
> variables. The user still has the final say via the KCPPFLAGS, KAFLAGS
> and KCFLAGS variables.
>
> Reported-by: Vineet Gupta <[email protected]>
> Signed-off-by: Michal Marek <[email protected]>

Sweet, that works for me with the following patch below.

Some logistics things:
- It would be nice to keep both of these patches together - do u want to do
that or want me to pick this one up
- For ARC this fixes a regression starting 3.16 - so I'll need a stable backport
which but obviously requires above to go to stable. Do u have any issues with
that. Shall we do the stable request after these hit the mainline...

------------->
>From 5458aa05ca3b2c57b683a27ce8226ab5029b9686 Mon Sep 17 00:00:00 2001
From: Vineet Gupta <[email protected]>
Date: Thu, 18 Jun 2015 13:54:01 +0530
Subject: [PATCH] ARC: Override toplevel default -O2 with -O3

ARC kernels have historically been built with -O3, despite top level
Makefile defaulting to -O2. This was facilitated by implicitly ordering
of arch makefile include AFTER top level assigned -O2.

An upstream fix to top level a1c48bb160f ("Makefile: Fix unrecognized
cross-compiler command line options") changed the ordering, making ARC
-O3 defunt.

Fix that by NOT relying on any ordering whatsoever and use the proper
arch override facility now present in kbuild (ARCH_*FLAGS)

Suggested-by: Michal Marek <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arc/Makefile b/arch/arc/Makefile
index 6107062c0111..46d87310220d 100644
--- a/arch/arc/Makefile
+++ b/arch/arc/Makefile
@@ -49,7 +49,8 @@ endif

ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
# Generic build system uses -O2, we want -O3
-cflags-y += -O3
+# Note: No need to add to cflags-y as that happens anyways
+ARCH_CFLAGS += -O3
endif

# small data is default for elf32 tool-chain. If not usable, disable it
--
1.9.1

2015-07-02 19:50:50

by Michal Marek

[permalink] [raw]
Subject: Re: ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

Dne 2.7.2015 v 07:27 Vineet Gupta napsal(a):
> On Wednesday 01 July 2015 08:49 PM, Michal Marek wrote:
>> On Wed, Jun 24, 2015 at 05:50:16PM +0530, Vineet Gupta wrote:
>>> On Thursday 18 June 2015 04:02 PM, Vineet Gupta wrote:
>>>> On Thursday 18 June 2015 03:44 PM, Michal Marek wrote:
>>>>> Uh, this is not what I meant. KCFLAGS is a *user* setting. It's meant to
>>>>> be set in the environment or command line.
>>>>
>>>> Well I don't want to rely on external settings whatsoever to enforce this. If this
>>>> is not the right way, what do u suggest I do to help fix this.
>>>
>>>
>>> Can I keep this seeming abuse of KCFLAGS or do u suggest alternate approach I can
>>> code up !
>>
>> Hi Vineet, sorry for the late reply.
>
> NP !
>
>> I had something like the below
>> patch in mind, simply allow arc to specify -O3 in ARCH_CFLAGS (that part
>> I'm leaving up to you).
>
> See below !
>
>> From e458fdf4ae37e1adce81b58d96b1075b4f0656e6 Mon Sep 17 00:00:00 2001
>> From: Michal Marek <[email protected]>
>> Date: Wed, 1 Jul 2015 17:13:16 +0200
>> Subject: [PATCH] kbuild: Allow arch Makefiles to override {cpp,ld,c}flags
>>
>> Since commit a1c48bb1 (Makefile: Fix unrecognized cross-compiler command
>> line options), the arch Makefile is included earlier by the main
>> Makefile, preventing the arc architecture to set its -O3 compiler
>> option. Since there might be more use cases for an arch Makefile to
>> fine-tune the options, add support for ARCH_CPPFLAGS, ARCH_AFLAGS and
>> ARCH_CFLAGS variables that are appended to the respective kbuild
>> variables. The user still has the final say via the KCPPFLAGS, KAFLAGS
>> and KCFLAGS variables.
>>
>> Reported-by: Vineet Gupta <[email protected]>
>> Signed-off-by: Michal Marek <[email protected]>
>
> Sweet, that works for me with the following patch below.
>
> Some logistics things:
> - It would be nice to keep both of these patches together - do u want to do
> that or want me to pick this one up

Feel free to pick up my patch.


> - For ARC this fixes a regression starting 3.16 - so I'll need a stable backport
> which but obviously requires above to go to stable. Do u have any issues with
> that. Shall we do the stable request after these hit the mainline...

Or just add

Cc: [email protected] # 3.16+

to the patches.

Michal

2015-07-03 02:58:39

by Vineet Gupta

[permalink] [raw]
Subject: Re: ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

On Friday 03 July 2015 01:20 AM, Michal Marek wrote:
>>> >> Reported-by: Vineet Gupta <[email protected]>
>>> >> Signed-off-by: Michal Marek <[email protected]>
>> >
>> > Sweet, that works for me with the following patch below.
>> >
>> > Some logistics things:
>> > - It would be nice to keep both of these patches together - do u want to do
>> > that or want me to pick this one up
>
> Feel free to pick up my patch.

Ok !

>> > - For ARC this fixes a regression starting 3.16 - so I'll need a stable backport
>> > which but obviously requires above to go to stable. Do u have any issues with
>> > that. Shall we do the stable request after these hit the mainline...
>
> Or just add
>
> Cc: [email protected] # 3.16+

Sure, I just wanted to ensure stable backport was OK for top level Makefile.

I just tried to apply it to 3.17.8 and and it doesn't thus will likely get dropped
by stable folks without a fixed up version.

I've added a "Depends-on" tag to ARC patch just in case.

-Vineet