[PATCH] powerpc: Always use -mno-string

[PATCH] powerpc: Always use -mno-string

[Benjamin Herrenschmidt]

The string load/store instructions are causing more damage than
help on a lot of processors where they are microcoded and are
generally not good for the kernel to use. Let's make sure we
don't emit them on any powerpc variant.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Ok, let's keep load/store multiple, they are only used on 32 bits
and are actually useful. string instructions are not though.

Index: linux-cell/arch/powerpc/Makefile
===================================================================
--- linux-cell.orig/arch/powerpc/Makefile	2007-03-22 17:02:42.000000000 +1100
+++ linux-cell/arch/powerpc/Makefile	2007-03-22 17:19:28.000000000 +1100
@@ -102,9 +102,9 @@ CFLAGS += $(call cc-option,-mno-altivec)
 # kernel considerably.
 CFLAGS += $(call cc-option,-funit-at-a-time)
 
-ifndef CONFIG_FSL_BOOKE
-CFLAGS		+= -mstring
-endif
+# Never use string load/store instructions as they are
+# often slow when they are implemented at all
+CFLAGS		+= -mno-string
 
 ifeq ($(CONFIG_6xx),y)
 CFLAGS		+= -mcpu=powerpc

Re: [PATCH] powerpc: Always use -mno-string

[Segher Boessenkool]

> Ok, let's keep load/store multiple, they are only used on 32 bits
> and are actually useful. string instructions are not though.

Sure they are.  They are used for 64-bit integers (and
structs) on lots of 32-bit code compiled by GCC.  As long
as they are 32-bit aligned, they aren't slow on e.g. PPC750
either.

Just tell GCC what CPU model you want to compile for and
it will try to do the best thing.  Don't actively work
against it :-)


Segher

[PATCH] force -mno-string option on cell

[Akinobu Mita]

GCC may generate inline copy loop to handle memcpy() function
instead of kernel defined memcpy() with -mstring or -Os.
But this inlined version of memcpy() caused an alignment interrupt
when copying from SPU local store on PS3.
(http://ozlabs.org/pipermail/cbe-oss-dev/2007-March/001348.html)

This patch deletes -mstring option on all powerpc and force -mno-string
option on Cell.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Akinobu Mita <mita@fixstars.com>

---
 arch/powerpc/Makefile |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: 2.6-git-ps3/arch/powerpc/Makefile
===================================================================
--- 2.6-git-ps3.orig/arch/powerpc/Makefile
+++ 2.6-git-ps3/arch/powerpc/Makefile
@@ -102,8 +102,9 @@ CFLAGS += $(call cc-option,-mno-altivec)
 # kernel considerably.
 CFLAGS += $(call cc-option,-funit-at-a-time)
 
-ifndef CONFIG_FSL_BOOKE
-CFLAGS		+= -mstring
+# No string instruction for SPE local store areas
+ifeq ($(CONFIG_PPC_CELL),y)
+CFLAGS		+= -mno-string
 endif
 
 ifeq ($(CONFIG_6xx),y)

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

> GCC may generate inline copy loop to handle memcpy() function
> instead of kernel defined memcpy() with -mstring or -Os.
> But this inlined version of memcpy() caused an alignment interrupt
> when copying from SPU local store on PS3.
> (http://ozlabs.org/pipermail/cbe-oss-dev/2007-March/001348.html)
>
> This patch deletes -mstring option on all powerpc and force -mno-string
> option on Cell.

Please split into two patches, the first half (deleting
-mstring) we can all agree on and "the other half".

What your patch does is setting -mno-string for all files
if your kernel is configured to support Cell.  It would be
better to set this option (and all other options needed,
-mno-multiple and -mno-algebraic come to mind, and that last
compiler option doesn't even exist yet) unconditionally on
the few Cell-specific files that are affected, not on the
whole kernel:

	CFLAGS_whatever.o := -msome-flags

It would be even better to not lie to the compiler by
telling it it can use the LS area as normal memory, since
evidently it cannot :-)


Segher

Re: [PATCH] force -mno-string option on cell

[Benjamin Herrenschmidt]

On Fri, 2007-03-23 at 13:56 +0100, Segher Boessenkool wrote:
> > GCC may generate inline copy loop to handle memcpy() function
> > instead of kernel defined memcpy() with -mstring or -Os.
> > But this inlined version of memcpy() caused an alignment interrupt
> > when copying from SPU local store on PS3.
> > (http://ozlabs.org/pipermail/cbe-oss-dev/2007-March/001348.html)
> >
> > This patch deletes -mstring option on all powerpc and force -mno-string
> > option on Cell.
> 
> Please split into two patches, the first half (deleting
> -mstring) we can all agree on and "the other half".
> 
> What your patch does is setting -mno-string for all files
> if your kernel is configured to support Cell.  It would be
> better to set this option (and all other options needed,
> -mno-multiple and -mno-algebraic come to mind, and that last
> compiler option doesn't even exist yet) unconditionally on
> the few Cell-specific files that are affected, not on the
> whole kernel:
> 
> 	CFLAGS_whatever.o := -msome-flags
> 
> It would be even better to not lie to the compiler by
> telling it it can use the LS area as normal memory, since
> evidently it cannot :-)

It's fair enough to use -mno-string overall. If the kernel is going to
run on cell, performance will suck with string instructions. In fact,
I'm not sure they are worth having on anything anyway.

Ben.

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

> It's fair enough to use -mno-string overall.

> If the kernel is going to
> run on cell, performance will suck with string instructions.

Yes.

> In fact,
> I'm not sure they are worth having on anything anyway.

We'll have to disagree here.  String insns _are_ useful
on many CPUs.

Either way, even if string insns would suck on every
CPU -- this shouldn't be handled in the kernel Makefiles,
convince the GCC people instead.  If there are no special
considerations why certain insns would be inappropriate
for a certain specific piece of code, you'll just have
to trust the compiler's insn selection.  That's just the
same as for any other isns patterns.

If there are such considerations, add the flags you need
for that source file, and that source file only.

If you don't compile targeting a specific CPU, and GCC's
choices suck on the hardware you actually run on, select
a different cost model.  Or, again, convince the GCC people
to make some changes.


Segher

Re: [Cbe-oss-dev] [PATCH] force -mno-string option on cell

[Arnd Bergmann]

On Friday 23 March 2007, Segher Boessenkool wrote:

> =A0=A0=A0=A0=A0=A0=A0=A0CFLAGS_whatever.o :=3D -msome-flags
>=20
> It would be even better to not lie to the compiler by
> telling it it can use the LS area as normal memory, since
> evidently it cannot :-)

Yes, this is the important point. I actually think the first
patch that just replaces memcpy with memcpy_fromio is the
right solution for the specific problem.

Avoiding certain instructions in the kernel may be a good
thing to do as well, but this is not at all a cell specific
thing.

Maybe we should have a more detailed CPU selection Kconfig
option like

CPU Family
   * 64 bit common (power3/4/5/6, ppc970, cell, *star)
   * 32 bit common (6xx, 82xx, 83xx, 86xx)
   * 40x/44x
   * ...

Minimum supported CPU (gcc -march=3D, depending on above selection)
   * 603
   * 604
   * 750
   * power3
   * power4
   * 970

Optimize for CPU (gcc -mtune)
   (subset of the -march list as before)

	Arnd <><

Re: [Cbe-oss-dev] [PATCH] force -mno-string option on cell

[Segher Boessenkool]

>> It would be even better to not lie to the compiler by
>> telling it it can use the LS area as normal memory, since
>> evidently it cannot :-)
>
> Yes, this is the important point. I actually think the first
> patch that just replaces memcpy with memcpy_fromio is the
> right solution for the specific problem.

Not only is it the _right_ solution, it actually _is_ a
solution -- adding -mno-string only sweeps one particular
symptom under the rug, it doesn't fix anything.

> Avoiding certain instructions in the kernel may be a good
> thing to do as well,

Only if there is something special about the kernel wrt
those instructions.  For example, if certain insns cannot
execute in supervisor mode on some CPU.

If there is no such special consideration, you should
just let GCC do its job (and if you think it makes bad
insn selection choices, you know where to come complain).

> but this is not at all a cell specific
> thing.

Indeed.

> Maybe we should have a more detailed CPU selection Kconfig
> option like
>
> CPU Family
>    * 64 bit common (power3/4/5/6, ppc970, cell, *star)
>    * 32 bit common (6xx, 82xx, 83xx, 86xx)
>    * 40x/44x
>    * ...

Useful.  This could at least partly be derived / defaulted
from the platform support that is already selected, too.

> Minimum supported CPU (gcc -march=, depending on above selection)
>    * 603
>    * 604
>    * 750
>    * power3
>    * power4
>    * 970

There is no -march= option.  You mean -mcpu= I think?

> Optimize for CPU (gcc -mtune)
>    (subset of the -march list as before)

-mcpu= implies -mtune= for the same CPU.  I don't think
many people would ever want to set something else.

Btw, when you install GCC, you can select its default
-mcpu= target: ../gcc/configure --with-cpu=970 .  It
helps ;-)


Segher

Re: [PATCH] force -mno-string option on cell

[Paul Mackerras]

Segher Boessenkool writes:

> What your patch does is setting -mno-string for all files
> if your kernel is configured to support Cell.  It would be
> better to set this option (and all other options needed,
> -mno-multiple and -mno-algebraic come to mind, and that last
> compiler option doesn't even exist yet) unconditionally on
> the few Cell-specific files that are affected, not on the
> whole kernel:
> 
> 	CFLAGS_whatever.o := -msome-flags

Unless someone can show a measurable difference in performance between
a kernel compiled with -mno-string and one compiled without (with the
kernel compiled without being faster), then I would be inclined just
to add -mno-string everywhere, since there are some cpus (e.g. e500
cores) where string instructions aren't implemented.

Paul.

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

>> What your patch does is setting -mno-string for all files
>> if your kernel is configured to support Cell.  It would be
>> better to set this option (and all other options needed,
>> -mno-multiple and -mno-algebraic come to mind, and that last
>> compiler option doesn't even exist yet) unconditionally on
>> the few Cell-specific files that are affected, not on the
>> whole kernel:
>>
>> 	CFLAGS_whatever.o := -msome-flags
>
> Unless someone can show a measurable difference in performance between
> a kernel compiled with -mno-string and one compiled without (with the
> kernel compiled without being faster), then I would be inclined just
> to add -mno-string everywhere, since there are some cpus (e.g. e500
> cores) where string instructions aren't implemented.

So add it _if_ support for e500 is compiled in.

Not using string instructions probably won't hurt performance
much, but adding -mno-string always doesn't solve any actual
problems -- in particular, it doesn't solve the original problem
that got all this started, which needs -mno-multiple too (and
that hurts performance on many CPUs) and -mno-algebraic and
-mno-dcbz (and those two options don't even exist yet).

If you decide to put -mno-string in there always anyway, please
put a nice fat big comment on there saying why this is done?


Segher

Re: [PATCH] force -mno-string option on cell

[Kumar Gala]

On Mar 23, 2007, at 7:06 AM, Akinobu Mita wrote:

> GCC may generate inline copy loop to handle memcpy() function
> instead of kernel defined memcpy() with -mstring or -Os.
> But this inlined version of memcpy() caused an alignment interrupt
> when copying from SPU local store on PS3.
> (http://ozlabs.org/pipermail/cbe-oss-dev/2007-March/001348.html)
>
> This patch deletes -mstring option on all powerpc and force -mno- 
> string
> option on Cell.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Akinobu Mita <mita@fixstars.com>

Nack.  If GCC is now putting string instructions even if you don't  
specify -mstring than we also need this for CONFIG_FSL_BOOKE.  Please  
fix that up as well.

- k

> ---
>  arch/powerpc/Makefile |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Index: 2.6-git-ps3/arch/powerpc/Makefile
> ===================================================================
> --- 2.6-git-ps3.orig/arch/powerpc/Makefile
> +++ 2.6-git-ps3/arch/powerpc/Makefile
> @@ -102,8 +102,9 @@ CFLAGS += $(call cc-option,-mno-altivec)
>  # kernel considerably.
>  CFLAGS += $(call cc-option,-funit-at-a-time)
>
> -ifndef CONFIG_FSL_BOOKE
> -CFLAGS		+= -mstring
> +# No string instruction for SPE local store areas
> +ifeq ($(CONFIG_PPC_CELL),y)
> +CFLAGS		+= -mno-string
>  endif
>
>  ifeq ($(CONFIG_6xx),y)
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

> Nack.  If GCC is now putting string instructions even if you don't 
> specify -mstring than we also need this for CONFIG_FSL_BOOKE.  Please 
> fix that up as well.

In the absence of -mstring and -mno-string options,
GCC will default to either on or off based on other
options (CPU model, mostly).  This is good behaviour
since it intends to give you the best code sequences
for whatever CPU you selected (or for a blended CPU
model etc).

This isn't new GCC behaviour at all.

If a specific piece of code really needs to be
generated without string insns, you should set the
option in the CFLAGS for that code only.

I don't know what the issue is for FSL BookE; if those
CPUs cannot run string insns in supervisor mode, then
you should indeed have -mno-string globally -- but that
is not the case AFAICS.


Segher

Re: [PATCH] force -mno-string option on cell

[Benjamin Herrenschmidt]

On Fri, 2007-03-23 at 17:35 +0100, Segher Boessenkool wrote:
> > Nack.  If GCC is now putting string instructions even if you don't 
> > specify -mstring than we also need this for CONFIG_FSL_BOOKE.  Please 
> > fix that up as well.
> 
> In the absence of -mstring and -mno-string options,
> GCC will default to either on or off based on other
> options (CPU model, mostly).  This is good behaviour
> since it intends to give you the best code sequences
> for whatever CPU you selected (or for a blended CPU
> model etc).

strings instructions suck, get away with it.

Ben.

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

>> In the absence of -mstring and -mno-string options,
>> GCC will default to either on or off based on other
>> options (CPU model, mostly).  This is good behaviour
>> since it intends to give you the best code sequences
>> for whatever CPU you selected (or for a blended CPU
>> model etc).
>
> strings instructions suck, get away with it.

No they don't.

End of this particular subthread I suppose, heh.


Segher

Re: [PATCH] force -mno-string option on cell

[Geert Uytterhoeven]

On Sat, 24 Mar 2007, Segher Boessenkool wrote:
> >> In the absence of -mstring and -mno-string options,
> >> GCC will default to either on or off based on other
> >> options (CPU model, mostly).  This is good behaviour
> >> since it intends to give you the best code sequences
> >> for whatever CPU you selected (or for a blended CPU
> >> model etc).
> >
> > strings instructions suck, get away with it.
> 
> No they don't.
> 
> End of this particular subthread I suppose, heh.

Really? `They suck!' 'They don't!'...

Ben: Why do they suck?
Segher: Why don't they suck?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH] force -mno-string option on cell

[Segher Boessenkool]

>>> strings instructions suck, get away with it.
>>
>> No they don't.
>>
>> End of this particular subthread I suppose, heh.
>
> Really? `They suck!' 'They don't!'...
>
> Ben: Why do they suck?
> Segher: Why don't they suck?

On most CPUs, they perform just fine, microcoded or not,
in most circumstances (hint: the bottleneck is not at the
decode stage).  They can actually help because of improved
code density.

For CPUs where they do suck, GCC knows (or should know)
preferably not to generate them.  Like on CBE, where lswi
causes a lovely 11-cycle bubble.  But guess what, many
more insns do so -- and we're not going to unconditionally
add a compiler flag to every kernel build that prevents
the compiler from generating "andi." insns I hope :-)


Segher