Add support for /Ob1 (and equivalent -finline-hint-functions), which enable inlining only for functions marked inline, either explicitly (via inline keyword, for example), or implicitly (function definition in class body, for example).
This works by enabling inlining pass, and adding noinline attribute to every function not marked inline.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Can't we implement /Ob1 by applying noinline to every non-inline specified function with weak linkage?
Comment Actions
I don't think weak linkage defines this.
For example, in:
struct A {
inline int foo() { return 0; }
int bar() { return 1; }
int baz();
};
int A::baz() { return 2; }With /Ob1, we should inline foo and bar, but never baz. But baz doesn't have weak linkage, does it?
But indeed, if in CodeGenFunctions.cpp, instead of adding InlineHint attribute when we have isInlined(), I add noinline attribute if we don't have it (with /Ob1 or equivalent clang flag), I could avoid the work I started to inline only hinted functions at http://reviews.llvm.org/D20603
Comment Actions
Right, sorry, normal external linkage is the obvious case. I was hung up thinking about cases like this:
volatile int x;
template <typename T>
struct A {
void f();
void g() { ++x; } // implicitly inline
};
template <typename T>
void A<T>::f() { ++x; } // not inline
int main() {
A<int> a;
a.f(); // not inlined with /Ob1
a.g(); // inlined with /Ob1
}In this code, A::g has weak linkage but is not inline. MSVC will not inline it with /Ob1.
Can you get the behavior you want by doing if (!FD->isInlined()) Fn->addFnAttr(llvm::Attribute::NoInline); instead? It seems nicer because we don't need to thread any options to the inliner, like you did in D20603.
Comment Actions
I'm still bemused that Clang thinks of a defined-in-class method and a method marked with 'inline' as significantly different things. But given that it does, a way to make them be treated the same will be nice.
| include/clang/Driver/Options.td | ||
|---|---|---|
| 751 | Seems like a HelpText would be appropriate here? | |
Comment Actions
New patch, which instead add NoInline to function not marked (implicitly or explicitly) inline, when we use the -finline-hint-functions flag (effectily inlining only implicitly and explicitly inline functions). Also adds support for /Ob1, since it's now an alias for it.
There is no longer a flag to treat implicit and explicit inline functions exactly the same way, I don't kow if it's really useful (it can be if we want to use the threshold difference between InlineHint and standard functions). If it's wanted for other uses, I can re-add it.
There is no flag to inline only explicitly marked inline functions, but it can be done by passing -finline-functions -mllvm -inline-threshold=-1000 (which will add InlineHint only to explicitly inline functions, and put a negative enough threshold for non-hinted functions to inline only the hinted ones).
Comment Actions
No, that's fine, if we decide we care deeply enough we can do our own patch. I mentioned it mainly as a curiosity.
Comment Actions
lgtm
Another consequence of doing it this way is that the /Ob1 decision will survive through LTO, for better or worse.
If we want to treat them the same, IMO we should have an option to drop inlinehint. IMO the inliner shouldn't treat any of these any differently:
inline void f() {} // explicitly inline
struct Foo {
void f() {} // implicitly inline
};
template <typename T> struct Bar { void f(); };
template <typename T> void Bar<T>::f() {} // weak linkage but not inline| test/Driver/cl-options.c | ||
|---|---|---|
| 283 | You're removing it from the ignored flags here, but I don't see any code change or test for how clang-cl's supposed to handle it. What am I missing? | |
| test/Driver/cl-options.c | ||
|---|---|---|
| 283 | Removing it from the ignored flags make if fallback to the generic /0 multi-param handling, in MSVCToolchain.cpp TranslateOptArg function. | |
Seems like a HelpText would be appropriate here?