Page MenuHomePhabricator

[Inliner] Attribute callsites with inline remarks
ClosedPublic

Authored by yrouban on Aug 8 2018, 4:08 AM.

Details

Summary

Sometimes reading an output *.ll file it is not easy to understand why some callsites are not inlined. We can read output of inline remarks (option --pass-remarks-missed=inline) and try correlating its messages with the callsites.

An easier way proposed by this patch is to add to every callsite processed by Inliner an attribute with the latest message that describes the cause of not inlining this callsite. The attribute is called inline-remark. By default this feature is off. It can be switched on by the option -inline-remark-attribute.

For example in the provided test the result method @test1 has two callsites @bar and inline remarks report different inlining missed reasons:

remark: <unknown>:0:0: bar not inlined into test1 because too costly to inline (cost=-5, threshold=-6)
remark: <unknown>:0:0: bar not inlined into test1 because it should never be inlined (cost=never): recursive

It is not clear which remark correspond to which callsite. With the inline remark attribute enabled we get the reasons attached to their callsites:

define void @test1() {
  call void @bar(i1 true) #0
  call void @bar(i1 false) #2
  ret void
}
attributes #0 = { "inline-remark"="(cost=-5, threshold=-6)" }
..
attributes #2 = { "inline-remark"="(cost=never): recursive" }

Patch by: yrouban (Yevgeny Rouban)

Diff Detail

Event Timeline

yrouban created this revision.Aug 8 2018, 4:08 AM
xbolva00 added inline comments.Aug 12 2018, 4:40 AM
test/Transforms/Inline/inline-remark.ll
2

Fix this line please (spaces..)

yrouban marked an inline comment as done.Aug 12 2018, 10:48 AM
yrouban updated this revision to Diff 160267.Aug 12 2018, 11:01 AM

fixed the test: removed extra whitespaces, changed description of Test1

xbolva00 accepted this revision.EditedAug 16 2018, 9:45 PM

looks good but wait also for @tejohnson 's approval

This revision is now accepted and ready to land.Aug 16 2018, 9:45 PM

A few comments/questions.

lib/Transforms/IPO/Inliner.cpp
119

Add a cl::desc for option.

510

LLVM coding style prefers early return to code nested in if body.

650

The comment here isn't very meaningful. Where does the OIC explain this? Please expand/replace comment (here and at line 1034).

679

I'm not sure what this ends up looking like - can you add a case with this string to your test?

1061

Why IR.message here but only IR online 673?

yrouban planned changes to this revision.Aug 17 2018, 12:34 PM

I will address all comments by the end of the next week.

yrouban marked 4 inline comments as done.Aug 22 2018, 1:31 AM
yrouban added inline comments.
lib/Transforms/IPO/Inliner.cpp
679

You are right, I forgot to change the return type of InlineCallIfPossible() from bool to InlineResult.

yrouban updated this revision to Diff 161897.Aug 22 2018, 1:44 AM
  • added cl:desc to inline-remark-attribute option
  • changed return type of InlineCallIfPossible() from bool to InlineResult
  • added one more test to see InlineResult printed in inline-remark attribute
  • reformatted body of function setInlineRemark()
This revision is now accepted and ready to land.Aug 22 2018, 1:44 AM

ninja check-all? :)

Re-checking is in progress. I'm almost sure it will pass as inline-remark-attribute is set to false by default.

make check-all passed.

make check-all passed.

Thanks. Any final words from @tejohnson ?

xbolva00 edited the summary of this revision. (Show Details)Aug 24 2018, 9:28 AM
This revision was automatically updated to reflect the committed changes.
yrouban updated this revision to Diff 162607.Aug 26 2018, 8:36 PM

Fixed build failure by moving operator<<(std::basic_ostream<char> &R, const ore::NV &Arg) out of #ifndef NDEBUG condition.
Started ninja check-all for non-debug config.

ninja check-all passed except one test failure Clang :: Modules/prune.m.
It seems to be unrelated to this patch as it is reproducible with clean master sources.

ninja check-all passed except one test failure Clang :: Modules/prune.m.
It seems to be unrelated to this patch as it is reproducible with clean master sources.

Thanks, I will try to recommit it later this day.

Thank you Dávid for your help.

Hi @yrouban,

Since you did some work in this area, can you look at this bug report please?

https://bugs.llvm.org/show_bug.cgi?id=42084

yrouban reopened this revision.Jun 5 2019, 4:52 AM

Hi @yrouban,
Since you did some work in this area, can you look at this bug report please?
https://bugs.llvm.org/show_bug.cgi?id=42084

Interesting. I will try but next week.

This revision is now accepted and ready to land.Jun 5 2019, 4:52 AM
xbolva00 closed this revision.Jun 10 2019, 2:58 PM