This is an archive of the discontinued LLVM Phabricator instance.

Enrich inline messages
ClosedPublic

Authored by yrouban on Jul 16 2018, 10:10 PM.

Details

Summary

This patch improves Inliner to provide causes/reasons for negative inline decisions.

  1. It adds one new message field to InlineCost to report causes for Always and Never instances. All Never and Always instantiations must provide a simple message.
  2. Several functions that used to return the inlining results as boolean are changed to return InlineResult which carries the cause for negative decision.
  3. Changed remark priniting and debug output messages to provide the additional messages and related inline cost.
  4. Adjusted tests for changed printing.

Patch by: yrouban (Yevgeny Rouban)

Diff Detail

Event Timeline

yrouban created this revision.Jul 16 2018, 10:10 PM
yrouban added a subscriber: llvm-commits.

This would be useful for my own analysis of missed inlines. A few nits.

include/llvm/Transforms/Utils/Cloning.h
219

I think this would make more sense in InlineCost.h

lib/Transforms/IPO/Inliner.cpp
430–431

nit: suggest making this:
<< " because it should never be inlined (" << IC << ")";
so that there is still a "(" delimiter before the "cost=never".

442

nit: suggest changing to:

<< NV("Caller", Caller) << " because too costly to inline with " << IC;

so that you don't end up with 2 levels of nested parentheses in messages.

Eugene.Zelenko set the repository for this revision to rL LLVM.
yrouban marked an inline comment as done.Jul 18 2018, 12:31 AM
yrouban added inline comments.
lib/Transforms/IPO/Inliner.cpp
442

do you mind if I put ':' before inline cost in both cases?

<< " because it should never be inlined: " << IC;
<< " because too costly to inline: " << IC;
yrouban marked an inline comment as not done.Jul 18 2018, 12:43 AM
yrouban added inline comments.
include/llvm/Transforms/Utils/Cloning.h
219

InlineCost.h relates to Analysis but InlineResult is a result of a transformations.
In other words all users of InlineResult reside in lib/Transforms.
But I do not insist.

yrouban updated this revision to Diff 156222.Jul 19 2018, 3:05 AM
yrouban marked an inline comment as not done.

Updated delimiters in output as requested.

tejohnson added inline comments.Jul 25 2018, 7:35 AM
include/llvm/Transforms/Utils/Cloning.h
219

It seems to mostly be related to analysis though, so Cloning.h seems to be the wrong place IMO. Mostly this structure is created by InlineCost.cpp (although I see it is also created by InlineFunction.cpp in some cases).

Also, I believe it is wrong to include a Transforms header into an Analysis cpp file, whereas the reverse is fine (I just checked and this would be the first Transforms header in any Analysis cpp file).

lib/Transforms/IPO/Inliner.cpp
442

Colon looks good here in the too costly to inline case. Slight preference for the parentheses in the earlier case, just around the "cost=never", to avoid the 2 sets of ":". I.e. so that it looks like:
foz not inlined into bar because it should never be inlined (cost=never): noinline function attribute (hotness: 30)

yrouban updated this revision to Diff 157666.Jul 27 2018, 5:08 AM
  1. formatted
  2. moved InlineResult from Cloning.h to InlineCost.h
  3. changed InlineCost printing to (cost=X, threshold=Y) or (cost=never): message or (cost=always): message. Had to fix many test checks.
nhaehnle removed a subscriber: nhaehnle.Jul 27 2018, 6:35 AM
This revision is now accepted and ready to land.Jul 29 2018, 8:46 AM
xbolva00 added inline comments.
lib/Transforms/IPO/Inliner.cpp
372

stringstream?

yrouban marked 2 inline comments as done.Jul 29 2018, 9:53 PM
yrouban updated this revision to Diff 157911.Jul 29 2018, 9:59 PM

Minor change: got got rid of multiple string operators += in inlineCostStr() by reusing the output stream template for printing InlineCost.

xbolva00 accepted this revision.Jul 29 2018, 10:03 PM

I have just made checked with the latest trunk.
Committers! could anyone land this patch please?

xbolva00 edited the summary of this revision. (Show Details)Jul 31 2018, 7:23 AM
This revision was automatically updated to reflect the committed changes.

I have just made checked with the latest trunk.
Committers! could anyone land this patch please?

Sadly, bot fails
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/27265/steps/ninja%20build%20local/logs/stdio

yrouban updated this revision to Diff 158463.Jul 31 2018, 10:13 PM

Fixed the buildbot failure (clang++ compilation error) by putting the operator<<(.., NV &Arg) before the template operator<<(.., InlineCost) in Inliner.cpp.

Fixed the buildbot failure (clang++ compilation error) by putting the operator<<(.., NV &Arg) before the template operator<<(.., InlineCost) in Inliner.cpp.

Re-landed

xbolva00 added a comment.EditedAug 1 2018, 1:01 AM

Clang and lld tests fail
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/34124

Please update them, run ninja check-all

The fixes seem to be simple, but I need to set up and build additional projects.
How can I submit patches to several projects that should be synchronously updated?

xbolva00 added a comment.EditedAug 2 2018, 12:15 AM

The fixes seem to be simple, but I need to set up and build additional projects.
How can I submit patches to several projects that should be synchronously updated?

Just prepare patches for Clang and LLD, We merge them + this one at the same time (some bot failures are expected, but after some time it should be okay)

And.. Set this patch as dependency for Clang and LLD patches :)

xbolva00 added a comment.EditedAug 5 2018, 7:55 AM

Ran ninja check-all with all patches - okay. So committed again. Not sure how bots/merge systems watch for dependencies, maybe I get some bot failures but after some time it should stabilize.

Bots are fine :) Thanks!

Thank you very much, Dávid!