subtle side effect of commit a1c48bb160f836

subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

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

Re: subtle side effect of commit a1c48bb160f836

[Geert Uytterhoeven]

Hi Vineet,

On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> 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 -- geert@linux-m68k.org

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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

On Thursday 18 June 2015 12:40 PM, Geert Uytterhoeven wrote:
> On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
> <Vineet.Gupta1@synopsys.com> 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

Re: subtle side effect of commit a1c48bb160f836

[Geert Uytterhoeven]

On Thu, Jun 18, 2015 at 9:33 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On Thursday 18 June 2015 12:40 PM, Geert Uytterhoeven wrote:
>> On Thu, Jun 18, 2015 at 8:47 AM, Vineet Gupta
>> <Vineet.Gupta1@synopsys.com> 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 -- geert@linux-m68k.org

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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

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-------

Re: subtle side effect of commit a1c48bb160f836

[Michal Marek]

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
> <Vineet.Gupta1@synopsys.com> 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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

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 <vgupta@synopsys.com>
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 <mmarek@suse.cz>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 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

Re: subtle side effect of commit a1c48bb160f836

[Geert Uytterhoeven]

Hi Vineet,

On Thu, Jun 18, 2015 at 10:45 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> 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 -- geert@linux-m68k.org

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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

+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
> <Vineet.Gupta1@synopsys.com> 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.

Re: subtle side effect of commit a1c48bb160f836

[Michal Marek]

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 <vgupta@synopsys.com>
> 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 <mmarek@suse.cz>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

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 <vgupta@synopsys.com>
>> > 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 <mmarek@suse.cz>
>> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> > Cc: <stable@vger.kernel.org>
>> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> > ---
>> >  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

Re: subtle side effect of commit a1c48bb160f836

[Vineet Gupta]

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 <vgupta@synopsys.com>
>>>> 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 <mmarek@suse.cz>
>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>> ---
>>>>  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

Re: subtle side effect of commit a1c48bb160f836

[Michal Marek]

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 <mmarek@suse.com>
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 <Vineet.Gupta1@synopsys.com>
Signed-off-by: Michal Marek <mmarek@suse.com>
---
 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

ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

[Vineet Gupta]

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 <mmarek@suse.com>
> 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 <Vineet.Gupta1@synopsys.com>
> Signed-off-by: Michal Marek <mmarek@suse.com>

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 <vgupta@synopsys.com>
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 <mmarek@suse.cz>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 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

Re: ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

[Michal Marek]

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 <mmarek@suse.com>
>> 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 <Vineet.Gupta1@synopsys.com>
>> Signed-off-by: Michal Marek <mmarek@suse.com>
> 
> 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: stable@vger.kernel.org # 3.16+

to the patches.

Michal

Re: ARC build -O3 (was Re: subtle side effect of commit a1c48bb160f836)

[Vineet Gupta]

On Friday 03 July 2015 01:20 AM, Michal Marek wrote:
>>> >> Reported-by: Vineet Gupta <Vineet.Gupta1@synopsys.com>
>>> >> Signed-off-by: Michal Marek <mmarek@suse.com>
>> > 
>> > 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: stable@vger.kernel.org # 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