powerpc: Always use -mno-string

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.

---

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

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

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.

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

Index: 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)

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

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.

Yes.

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

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

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.

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

Indeed.

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

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

-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

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.

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

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

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

strings instructions suck, get away with it.

Ben.

No they don't.

End of this particular subthread I suppose, heh.

Segher

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) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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