This is an archive of the discontinued LLVM Phabricator instance.

[Inliner] Report when inlining fails because callee's def is unavailable
ClosedPublic

Authored by anemet on Aug 11 2016, 11:53 AM.

Details

Summary

This is obviously an interesting case because it may motivate code
restructuring or LTO.

Reporting this requires instantiation of ORE in the loop where the call
sites are first gathered. I've checked compile-time
overhead *with* -Rpass-with-hotness and the worst slow-down was 6% in
mcf and quickly tailing off. As before without -Rpass-with-hotness
there is no overhead.

Because this could be a pretty noisy diagnostics, it is currently
qualified as 'verbose'. As of this patch, 'verbose' diagnostics are
only emitted with -Rpass-with-hotness, i.e. when the output is expected
to be filtered.

Diff Detail

Event Timeline

anemet updated this revision to Diff 67723.Aug 11 2016, 11:53 AM
anemet retitled this revision from to [Inliner] Report when inlining fails because callee's def is unavailable.
anemet updated this object.
anemet added reviewers: eraman, chandlerc, davidxl, hfinkel.
anemet added a subscriber: llvm-commits.
davide added a subscriber: davide.Aug 12 2016, 4:47 AM
davidxl added inline comments.
include/llvm/Analysis/OptimizationDiagnosticInfo.h
173

Is there a need for this wrapper?

lib/Transforms/IPO/Inliner.cpp
472

This message can be useful for thinLTO compilation, but for O2, I fear it can be quite noisy. Perhaps using another option to make it default?

anemet added inline comments.Aug 17 2016, 9:40 AM
include/llvm/Analysis/OptimizationDiagnosticInfo.h
173

Yes, it should be a pretty common use case to report both the fact that an opt failed and the reason for it. I am surprised we didn't need it so far (@hfinkel on IRC was actually surprised we didn't have such an API yet).

I guess one explanation that in the vectorizer analysis remarks are emitted inside the function checking legality along with the specific reasons while the missed-opt remark is emitted upon returning from the legality check function.

lib/Transforms/IPO/Inliner.cpp
472

Good point.

I think that eventually we will have to introduce a concept of accuracy/relevance/verbosity in these calls. These APIs are likely to go through some fundamental changes because the plan is to be able to gather all these remarks via YAML for post-processing.

For now, let me just experiment only reporting these when -Rpass-with-hotness is on. Then we know that we have a higher tolerance for noise because we're only looking at hot regions. This requires extending the API with verbosity parameter or something. @hfinkel, let me know if you have comments on this. Again, this is likely to be temporary but it's good to start having this notion in these APIs.

It would also be good to exclude reporting declarations from system headers. Any idea how to do that?

anemet updated this revision to Diff 68645.Aug 18 2016, 9:20 PM

Address David's comments.

I've added a new verbosity paramater to the ORE APIs and marked this new
inliner remark verbose. For now Verbose remarks are only emitted
if -Rpass-with-hotness is on, i.e. it's expected that the output would be
filtered.

anemet updated this object.Aug 18 2016, 9:22 PM

Ping. @davidxl, does the new version address your concerns?

davidxl added inline comments.Aug 26 2016, 10:00 AM
include/llvm/Analysis/OptimizationDiagnosticInfo.h
99

Perhaps change Verbose to IsVerbose or IsMsgVerbose

// If IsVerbose is true, the message is considered verbose and will only be emitted when verbose output is turned on

lib/Transforms/IPO/Inliner.cpp
472

verbose output is fine as long as there are ways for tools to do post-processing to filter/order messages according to importance. This is future work of course.

test/Transforms/Inline/optimization-remarks.ll
2

bad formatting?

anemet added inline comments.Aug 26 2016, 10:17 AM
include/llvm/Analysis/OptimizationDiagnosticInfo.h
99

IsVerbose WFM, thanks!

test/Transforms/Inline/optimization-remarks.ll
2

Why? I just wrapped the long line.

I usually do it like this: multiple RUN lines, separated with \, continuation lines indented.

anemet updated this revision to Diff 69405.Aug 26 2016, 11:05 AM

Rename Verbose to IsVerbose per David's comment.

davidxl accepted this revision.Aug 26 2016, 11:07 AM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Aug 26 2016, 11:07 AM
This revision was automatically updated to reflect the committed changes.