turn off gcc inliner from llvm-gcc

[lattner]

Bugzilla Link	2234
Resolution	FIXED
Resolved on	May 19, 2008 13:24
Version	unspecified
OS	All
CC	@asl,@regehr

Extended Description

Consider:
extern int bar ();
volatile int w;
int bar (void) {
if (bar())
return 0;
else
return w;
}

-O0 with opt -std-compile-opts compiles this to very reasonable code:

...
define i32 @​bar() nounwind {
entry:
%tmp1 = tail call i32 @​bar( ) nounwind ; [#uses=1]
%tmp2 = icmp eq i32 %tmp1, 0 ; [#uses=1]
br i1 %tmp2, label %bb4, label %UnifiedReturnBlock
bb4: ; preds = %entry
%tmp5 = volatile load i32* @​w, align 4 ; [#uses=1]
ret i32 %tmp5
UnifiedReturnBlock: ; preds = %entry
ret i32 0
}

When using llvm-gcc -O3 however, I get this crazy code:

define i32 @​bar() nounwind {
entry:
%tmp17 = tail call i32 @​bar( ) nounwind ; [#uses=1]
%tmp18 = icmp eq i32 %tmp17, 0 ; [#uses=1]
br i1 %tmp18, label %bb20, label %bb29
bb20: ; preds = %entry
%tmp21 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp103 = icmp eq i32 %tmp21, 0 ; [#uses=1]
br i1 %phitmp103, label %bb29, label %bb38
bb29: ; preds = %bb20, %entry
%tmp30 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp102 = icmp eq i32 %tmp30, 0 ; [#uses=1]
br i1 %phitmp102, label %bb38, label %bb47
bb38: ; preds = %bb29, %bb20
%tmp39 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp101 = icmp eq i32 %tmp39, 0 ; [#uses=1]
br i1 %phitmp101, label %bb47, label %bb56
bb47: ; preds = %bb38, %bb29
%tmp48 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp100 = icmp eq i32 %tmp48, 0 ; [#uses=1]
br i1 %phitmp100, label %bb56, label %bb65
bb56: ; preds = %bb47, %bb38
%tmp57 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp99 = icmp eq i32 %tmp57, 0 ; [#uses=1]
br i1 %phitmp99, label %bb65, label %bb74
bb65: ; preds = %bb56, %bb47
%tmp66 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp98 = icmp eq i32 %tmp66, 0 ; [#uses=1]
br i1 %phitmp98, label %bb74, label %bb83
bb74: ; preds = %bb65, %bb56
%tmp75 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp97 = icmp eq i32 %tmp75, 0 ; [#uses=1]
br i1 %phitmp97, label %bb83, label %bb92
bb83: ; preds = %bb74, %bb65
%tmp84 = volatile load i32* @​w, align 4 ; [#uses=1]
%phitmp = icmp eq i32 %tmp84, 0 ; [#uses=1]
br i1 %phitmp, label %bb92, label %UnifiedReturnBlock
bb92: ; preds = %bb83, %bb74
%tmp93 = volatile load i32* @​w, align 4 ; [#uses=1]
ret i32 %tmp93
UnifiedReturnBlock: ; preds = %bb83
ret i32 0
}

This is a miscompilation, because it causes multiple accesses to the volatile value, and seems pretty pointless in general.

[lattner]

incidentally, the testcase was from this paper: http://www.cs.utah.edu/~regehr/papers/emsoft08_submit.pdf

[llvmbot]

I don't think it's wrong; it's inlined the recursive call a few times, and each volatile load corresponds to a different execution of the function. It is, however, not a good idea. It may be gcc's inliner doing this.

[lattner]

Ah, you're quite right. I missed that this was a recursive call.

[lattner]

Ah, gcc -O3 also does this. :(

It looks like we need to throttle back the GCC inliner. Maybe we should switch it to only handle only-inline functions?

[lattner]

only-inline -> always-inline

[llvmbot]

On second thought, is it so clear this is a bad thing to do? -O3 is supposed to aim at max execution speed. This may be a win or not, depending on how many times the recursive call gets executed.

[lattner]

Independent of whether this specific case is good or not, I'd rather not have the GCC inliner be making strange decisions and then having the llvm inliner make other strange decisions :). If we want this case to be inlined, we can always make the llvm inliner do it.

Does that seem reasonable?

[llvmbot]

You mean, disable the gcc inliner completely? I have no problem with that, indeed I'm surprised it hasn't been done already.
We are very likely to have some performance problems to work through when it happens, though.

[lattner]

Yep, that is what I'm suggesting. The only case we care about is the 'early inliner' which handles attr(always_inline) functions. This thing needs to always run, even at -O0.

[llvmbot]

But why does gcc have to do it? Doesn't llvm's inliner honor that bit? It will have to for clang.

[llvmbot]

In any event I would not suggest tinkering with this before WWDC. There are always performance regressions when you touch the inlining heuristics.

[lattner]

Sounds fine with me.

[llvmbot]

Only gcc's early inliner is being run. The reason for running it is
that if performs exception handling simplifications that we are not
yet capable of. I don't mind turning it off though.

[lattner]

Ok, lets shoot for this for llvm 2.4, so do it right after 2.3 is released or branched.

[regehr]

Argh-- the recursiveness of this function is a typo in our submission! The code was supposed to be this:

extern int qux ();
volatile int a;
int baz (void) {
if (qux())
return 0;
else
return a;
}

It doesn't really matter since (1) both versions cause gcc-4.3 to emit buggy code and (2) neither version is miscompiled by LLVM.