Page MenuHomePhabricator

[Driver] Add support for -finline-functions and /Ob2 flags
ClosedPublic

Authored by Ilod on May 24 2016, 10:19 AM.

Details

Summary

-finline-functions and /Ob2 are currently ignored by Clang. The only way to enable inlining is to use the global O flags, which also enable other options, or to emit LLVM bitcode using Clang, then running opt by hand with the inline pass.
This patch allows to simply use the -finline-functions flag (same as GCC) or /Ob2 in clang-cl mode to enable inlining without other optimizations.
This is the first patch of a serie to improve support for the /Ob flags.

Diff Detail

Event Timeline

Ilod updated this revision to Diff 58261.May 24 2016, 10:19 AM
Ilod retitled this revision from to [Driver] Add support for -finline-functions and /Ob2 flags.
Ilod updated this object.
Ilod added a reviewer: hans.
Ilod added a subscriber: cfe-commits.
hans edited edge metadata.May 24 2016, 11:10 AM

Thanks! Some comments below:

lib/Driver/Tools.cpp
5335–5337

Please line break this; lines should be <= 80 columns wide.

5337

I think this can be:

InlineArg->render(Args, CmdArgs)

instead

lib/Frontend/CompilerInvocation.cpp
446

This line and 444 need line breaks too.

test/CodeGen/inline-optim.c
17

I'd suggest using "check-label" for the @foo checks. See http://llvm.org/docs/CommandGuide/FileCheck.html#the-check-label-directive

25

For all these checks, I'd suggest matching e.g.

"call i32 @inline_hint"

to make it a little easier to see what' being matched for. I suppose your checks could match either the calls, or the definitions of the called function.

Ilod updated this revision to Diff 58302.May 24 2016, 1:12 PM
Ilod edited edge metadata.

Thanks!

Updated to respect the 80-columns limit.
Updated the tests to have a clearer checks. (I don't know well the FileCheck, so I took example from test/CodeGen/noinline.c, but this is indeed more comprehensible).

hans accepted this revision.May 24 2016, 1:22 PM
hans edited edge metadata.

Looks good to me!

Do you have commit access, or would you like me to commit this for you?

This revision is now accepted and ready to land.May 24 2016, 1:22 PM
Ilod added a comment.May 24 2016, 1:24 PM

I don't have commit access, so you can do it for me, thanks.

This revision was automatically updated to reflect the committed changes.
hans added a subscriber: majnemer.May 24 2016, 2:31 PM

majnemer reminded me that /O options can be combined, so e.g. /Odb2 would be a valid combination. I've followed up with r270614 to move the /Ob flags into that mechanism.

Ilod added a comment.May 24 2016, 4:21 PM

Hello,

It seems the new test was commited as .cc instead of .c, which changes the name mangling, making // NOINLINE-LABEL: @foo fail.
Either the file should be renamed, or -x c should be added to clang invocation, or LABEL check updated.

hans added a comment.May 24 2016, 4:44 PM
In D20576#438581, @Ilod wrote:

It seems the new test was commited as .cc instead of .c, which changes the name mangling, making // NOINLINE-LABEL: @foo fail.
Either the file should be renamed, or -x c should be added to clang invocation, or LABEL check updated.

Sigh, I'm not getting many points today.

I renamed the test because when I tried it, the inline_hint() function didn't get inlined in C mode, but it did in C++ mode. But in so doing, I renamed it to .cc, which means the test didn't run at all (it should be .cpp), which is why I didn't notice that the test doesn't pass.

The reason inlining didn't happen was probably because I ran the test for a different target than you did. Which reminds me, the test needs to specify a target triple.

Hopefully I managed to make things right in r270633