This is an archive of the discontinued LLVM Phabricator instance.

Add support for /Ob1 and -finline-hint-functions flags
ClosedPublic

Authored by Ilod on May 25 2016, 3:40 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Ilod updated this revision to Diff 58523.May 25 2016, 3:40 PM
Ilod retitled this revision from to Add flag to add InlineHint attribute on implicitly inline functions.
Ilod updated this object.
Ilod added a subscriber: cfe-commits.
Ilod added a reviewer: rsmith.May 25 2016, 4:00 PM
Ilod updated this revision to Diff 61029.Jun 16 2016, 2:21 PM
Ilod added a reviewer: rnk.

Updated to head revision, and pinging this.

rnk edited edge metadata.Jun 16 2016, 3:36 PM

Can't we implement /Ob1 by applying noinline to every non-inline specified function with weak linkage?

Ilod added a comment.Jun 16 2016, 4:28 PM

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

rnk added a comment.Jun 17 2016, 8:27 AM
In D20647#460520, @Ilod wrote:

I don't think weak linkage defines this.

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.

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?

Ilod updated this revision to Diff 61318.Jun 20 2016, 4:21 PM
Ilod edited edge metadata.

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

In D20647#462797, @Ilod wrote:

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.

No, that's fine, if we decide we care deeply enough we can do our own patch. I mentioned it mainly as a curiosity.

Ilod retitled this revision from Add flag to add InlineHint attribute on implicitly inline functions to Add support for /Ob1 and -finline-hint-functions flags.Jun 21 2016, 5:10 AM
Ilod updated this object.
Ilod added a reviewer: hans.
rnk accepted this revision.Jun 21 2016, 10:36 AM
rnk edited edge metadata.

lgtm

Another consequence of doing it this way is that the /Ob1 decision will survive through LTO, for better or worse.

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.

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
This revision is now accepted and ready to land.Jun 21 2016, 10:36 AM
hans added inline comments.Jun 21 2016, 10:45 AM
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?

Ilod marked an inline comment as done.Jun 21 2016, 11:25 AM
Ilod added inline comments.
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.
However I just checked that I forgot to forward the argument from the clang driver to cc1 in ConstructJob. Will add this (and a test for the driver) and submit a new patch.

Ilod updated this revision to Diff 61418.Jun 21 2016, 12:16 PM
Ilod edited edge metadata.

Fixed passing argument from clang driver to cc1.
Added driver tests.

Ilod marked 2 inline comments as done.Jun 21 2016, 12:17 PM
hans accepted this revision.Jun 21 2016, 1:00 PM
hans edited edge metadata.
In D20647#463590, @Ilod wrote:

Fixed passing argument from clang driver to cc1.
Added driver tests.

lgtm, thanks!

Ilod added a comment.Jun 22 2016, 8:02 AM

Thanks.
I will let you submit it for me if everything seems ok.

This revision was automatically updated to reflect the committed changes.