GCC 3.4 and broken inlining.

Nigel Cunningham <[email protected]> writes:

I think a better solution would be to apply the appended patch

And then just mark the function you know needs to be inlined as __always_inline__. I did this on x86-64 for some functions too that need to be always inlined (although using the attribute directly because all x86-64 compilers support it)

The rationale is that the inlining algorithm in gcc 3.4+ is quite a lot better and actually eliminates some not-so-great inlining we had in the past and makes the kernel a bit smaller.

-Andi

P.S.: compiler.h seems to be not "gcc 4.0 safe". Probably that needs to be fixed too.

diff -u linux-2.6.7-bk9-work/include/linux/compiler.h-o linux-2.6.7-bk9-work/include/linux/compiler.h
@@ -120,4 +120,8 @@
 #define noinline
 #endif
 
+#ifndef __always_inline
+#define __always_inline
+#endif
+
 #endif /* __LINUX_COMPILER_H */

diff -u linux-2.6.7-bk9-work/include/linux/compiler-gcc3.h-o linux-2.6.7-bk9-work/include/linux/compiler-gcc3.h
@@ -9,6 +9,10 @@
 # define __inline	__inline__ __attribute__((always_inline))
 #endif
 
+#if __GNUC_MINOR__ >= 1
+# define __always_inline inline __attribute__((always_inline))
+#endif
+
 #if __GNUC_MINOR__ > 0
 # define __deprecated	__attribute__((deprecated))
 #endif

Hi.

I'm not sure what I wrote that you're replying to.

I'm going to be a pragmatist :> As long as it works. I do think that functions being declared inline when they can't be inlined is wrong, but there are more important things on which to spend my time.

Should that be __always_inline (no final __ in the patch below, so far as I can see)?

[...]

Yes.

Regards,

Nigel

It was just a general reply to the thread.

Originally as written surely. The problem we have in Linux is that people write some code, declare functions as inline and it usually makes sense. But then years pass and people hack and enhance the code and add more code to the inline functions. And even more code. But they usually don't drop the inline marker and move it out of headers when the function has become far too big to still be a good inlining candidate. So we end up with functions marked inlined that should not really be inlined.

One reason is probably that patches are rated for "intrusiveness" based on the number of lines they change and when you move an inline function out of a header even a small change can become quite big. That's a unfortunate side effect of a normally sound policy.

Anyways, with that problem and the improved inliner in gcc 3.4 I think it's a good idea to let the compiler decide.

It's too bad that i386 doesn't enable -funit-at-a-time, that improves the inlining heuristics greatly.

Yes. I originally wrote it with the final __, but it's better to not add it.

-AndI

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Andi Kleen <[email protected]>:

- From the gcc manpage:

- -O2 turns on all optimization flags specified by -O. It also turns on the following optimization flags: -fforce-mem
- -foptimize-sibling-calls -fstrength-reduce -fcse-follow-jumps
- -fcse-skip-blocks -frerun-cse-after-loop -frerun-loop-opt
- -fgcse -fgcse-lm -fgcse-sm -fgcse-las -fdelete-null-pointer-checks
- -fexpensive-optimizations -fregmove -fschedule-insns
- -fschedule-insns2 -fsched-interblock -fsched-spec -fcaller-saves
- -fpeephole2 -freorder-blocks -freorder-functions -fstrict-aliasing
- -funit-at-a-time -falign-functions -falign-jumps -falign-loops ^^^^^^^^^^^^^^^^
- -falign-labels -fcrossjumping

Do I miss something?

- --
Regards Michael Buesch [ http://www.tuxsoft.de.vu ]

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA7mjJFGK1OIvVOP4RAuXyAJ0fM5x1jqCZGkzO2jfl42CaNLgJJwCdGZQW 3rUgM3svqyWb8JaCavWAkhg=
=3LeQ
-----END PGP SIGNATURE-----

# gcc-3.4.1/gcc/opts.c

if (optimize >= 2)

    {
(...)
      flag_unit_at_a_time = 1;
    }

btw).

I *don't trust* manpages ;)

# man gcc

-fomit-frame-pointer

Don't keep the frame pointer in a register for functions that don't need one. This avoids the instructions to save, set up and restore frame pointers; it also makes an extra register available in many functions. It also makes debugging impossible on some machines.

               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(...)
Enabled at levels -O, -O2, -O3, -Os.
^^^^^^^

if (optimize >= 1)

    {
(...)
#ifdef CAN_DEBUG_WITHOUT_FP
      flag_omit_frame_pointer = 1;

#endif
(...)

finally, at ix86 -O[123s] doesn't turn on -fomit-frame-pointer. manpage tells somethine else...

/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)

The problem is not when they can or cannot be inlined. The inline keyword has nothing to do with that. It's a hint to the compiler, that means that inlining the function is likely to be profitable. But, like the register' keyword, it's just a hint. And, unlike the register' keyword, it doesn't make certain operations on objects marked with it ill-formed (e.g., you can't take the address of an register variable, but you can take the address of an inline function).

The issue with inlining that makes it important for the compiler to have something to say on the decision is that several aspects of the profit from expanding the function inline is often machine-dependent. It depends on the ABI (calling conventions), on how slow call instructions are, on how important instruction cache hits are, etc. Sure enough, GCC doesn't take all of this into account, so its heuristics sometimes get it wrong. But it's getting better.

Meanwhile, you should probably distinguish between must-inline, should-inline, may-inline, should-not-inline and must-not-inline functions. Attribute always_inline covers the must-inline case; the inline keyword covers the may-inline'case. The absence of the inline keyword implies `should-not-inline', and attribute noinline covers must-not-inline. should-inline can't be expressed today, and if may-inline doesn't get the desired effect, you may feel tempted to abuse always_inline, but I suggest you to do so using a different macro, such that, if GCC ever introduces a new attribute that's a stronger hint to inlining that doesn't error out if it can't be done, you can easily switch to it.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

The main reason always_inline was added is that gcc 3.3 stopped inlining copy_from/to_user, which generated horrible code bloat (because it consists of a lot of code that was supposed to be optimized away, and putting it in a static into every module generated a lot of useless code)

At this time the poor person blessed with this compiler y took the easy way out - just define inline as always_inline.

It may have been possible to do it only for selected functions, but that would have been a lot of work: you cannot really expect that the kernel goes through a large effort just to work around compiler bugs in specific compiler versions.

The gcc 3.4/3.5 inliner seem to be better, but is still quite bad in cases (e.g. 3.5 stopped to inline the three line fix_to_virt() which requires inlining). For 3.4/3.5 it's probably feasible to do this, but I doubt it is worth someone's time for 3.3.

gcc is extremly dumb at that currently. Linux has a lot of inline functions like

static inline foo(int arg)

{ 
	if (__builtin_constant_p(arg)) { 
		/* lots of code that checks for arg and does different things */
	} else { 
		/* simple code */
	}
} 

(e.g. take a look at asm/uaccess.h for extreme examples)

The gcc inliner doesn't know that all the stuff in the constant case will be optimized away and it assumes the function is big. That's really a bug in the inliner.

But even without that it seems to do badly - example is asm/fixmap.h:fix_to_virt()

#define __fix_to_virt(x) (FIXADDR_TOP - ((x) << PAGE_SHIFT))
static inline unsigned long fix_to_virt(const unsigned int idx)

{
        if (idx >= __end_of_fixed_addresses)
                __this_fixmap_does_not_exist();

        return __fix_to_virt(idx);
}

This three liner is _not_ inlined in current gcc mainline. I cannot describe this in any other way than badly broken.

You're asking us to do a lot of work just to work around compiler bugs?

I can see the point of having must-inline - that's so rare that it can be declared by hand. May inline is also done, except for a few misguided people who use -O3. should not inline seems like overkill.

-Andi

Not asking. Just suggesting that you make your request to the compiler clearer. This may enable the compiler to do a better job for you. You don't have to switch it all at once. Keep inline as always_inline, if you like, and downgrade other inline requests as you see fit.

Of course having inline expand to something containing always_inline will take a bit of preprocessor hackery to get other macros to expand to the inline keyword without this attribute.

`should not inline' is the default: a function not declared as inline won't be inlined unless several conditions are met, e.g., compiling with -O3 and/or -finline-all-functions. It's the other cases to tune inlining directives that would be useful.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Making the kernel a bit smaller is a small improvement.

Runtime errors caused with gcc 3.4 are IMHO much worse than such a small improvement or three dozen compile errors with gcc 3.4 .

The effect of your patch with gcc 3.4 would be nearly:
Ignore all old inlines and add something new that does what inline did before.

Wouldn't it be a better solution if you would audit the existing inlines in the kernel for abuse of inline and fix those instead?

My copy of compiler.h says:

<-- snip -->

#if __GNUC__ > 3

# include <linux/compiler-gcc+.h>       /* catch-all for GCC 4, 5, etc. */

<-- snip -->

What exactly is wrong with this?

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

What runtime errors?

Actually requiring inlining is extremly rare and such functions should get that an explicit always inline just for documentation purposes. (another issue is not optimized away checks, but that shows at link time)

In the x86-64 case it was vsyscalls, in Nigel's case it was swsusp. Both are quite exceptional in what they do.

I don't see any point in going through ~1.2MLOC of code by hand when a compiler can do it for me.

-Andi

First of all, your proposed patch seems to be broken WRT gcc < 3.1 .

How can a compiler decide whether an "inline" was for a possible small speed benefit or whether it's required for correct working?

And I'm not that happy with the fact that gcc 3.3 and gcc 3.4 will produce significantly different code for the same file. Besides from the 3 dozen compile errors I'm currently sorting out, gcc 3.3 and 3.4 should behave similar with __attribute__((always_inline)).

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

The latest version does fallback #define __always_inline inline That was indeed broken in the original patch.

It can't, but the 0.001% of needed for inlines that are required for correctness should be always marked __always_inline just for documentation alone.
You probably don't realize how special a case this is.

They are different compiler versiopns, they generate different code.

-Andi

Agreed.

It's probably a good idea to define such functions as `extern inline' (another GCC extension), such that a definition of the function is never emitted, and you get a linker error if the compiler somehow fails to emit an error on a failure to inline the function.

Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

That used to be done in the past for all functions, but it was stopped because gcc suddenly stopped inlining functions it did previously and it caused a lot of spurious linker errors.

I guess it could be readded if the inlining heuristics were fixed, but even in gcc 3.5 it still looks quite bleak.

-Andi

It's very simple. For use in the kernel we don't *want* any inlining heuristics. What we want is:

a) If the programmer says "inline", then inline it.

b) If the programmer didn't say "inline" then don't inline it.

Surely it is not hard to add a new option to gcc to provide these semantics?

The problem is that we have a lot of "stale" inlines. Inlines that made sense a long time ago, but then people added a lot more code to the function and it would be better to out line it again. You should know, you seem to do this kind of out-lining most ...

For those it may even make sense to let the compiler chose.

That option is -O2 -Dinline="__attribute__((always_inline))" But for some reason it was turned off for 3.4/3.5.

-Andi

We've already fixed zillions of those, and patches are accepted. I think someone wrote a tool to hunt those functions down, too.

We can see how far that's getting us ;)

Please tell me that was just a bug, and it will be fixed very soon.

Ok, we'll see. Fixing that would be good of course.

compiler-gcc3.h:

#if __GNUC_MINOR__ >= 1 && __GNUC_MINOR__ < 4

# define inline         __inline__ __attribute__((always_inline))
# define __inline__     __inline__ __attribute__((always_inline))
# define __inline       __inline__ __attribute__((always_inline))
#endif

iirc the problem was that gcc errors now out when a function marked like this cannot be inlined for some reason.

-Andi

It's a bug in compiler-gcc3.h, and I already sent the patch below in this thread.

I'm currently sending fixes for compile errors this causes with gcc 3.4 (I've forgotten the exact number, but there were compile errors in at about 30 files in a full i386 compile).

<-- snip -->

[patch] #define inline as __attribute__((always_inline)) also for gcc >= 3.4

Rationale:
- if gcc 3.4 can't inline a function marked as "inline" that's a strong hint that further investigation is required
- I strongly prefer a compile error over a potential runtime problem

@@ -3,7 +3,7 @@
 /* These definitions are for GCC v3.x.  */
 #include <linux/compiler-gcc.h>
 
-#if __GNUC_MINOR__ >= 1  && __GNUC_MINOR__ < 4
+#if __GNUC_MINOR__ >= 1
 # define inline		__inline__ __attribute__((always_inline))
 # define __inline__	__inline__ __attribute__((always_inline))
 # define __inline	__inline__ __attribute__((always_inline))

While we're there, shouldn't this really be the following?

# define inline          inline   __attribute__((always_inline))
# define __inline__    __inline__ __attribute__((always_inline))
# define __inline      __inline   __attribute__((always_inline))

I find it somewhat annoying that the preprocessor expands every "inline" to "__inline__ __attribute__((always_inline)) __attribute__((always_inline))" in the current code.

	Arnd <><

Sounds like a good idea.

Updated patch below.

<-- snip -->

[patch] #define inline as __attribute__((always_inline)) also for gcc >= 3.4

Rationale:
- if gcc 3.4 can't inline a function marked as "inline" that's a strong hint that further investigation is required
- I strongly prefer a compile error over a potential runtime problem

Additionally, it changes all #define's to expand to the correct {,__}inline{,__} (suggested by Arnd Bergmann <[email protected]>).

@@ -3,10 +3,10 @@
 /* These definitions are for GCC v3.x.  */
 #include <linux/compiler-gcc.h>
 
-#if __GNUC_MINOR__ >= 1  && __GNUC_MINOR__ < 4
-# define inline		__inline__ __attribute__((always_inline))
-# define __inline__	__inline__ __attribute__((always_inline))
-# define __inline	__inline__ __attribute__((always_inline))
+#if __GNUC_MINOR__ >= 1
+# define inline		inline		__attribute__((always_inline))
+# define __inline__	__inline__	__attribute__((always_inline))
+# define __inline	__inline	__attribute__((always_inline))
 #endif
 
 #if __GNUC_MINOR__ > 0