Page MenuHomePhabricator

[Inliner] Don't initialize ComputeFullInlineCost to be always true because of ORE
ClosedPublic

Authored by wmi on Feb 19 2019, 11:26 AM.

Details

Summary

Right now ComputeFullInlineCost is always true because it is set to be true if ORE is non-null. However ORE is never null for inliner. Whether we will emit optimization remark for inliner doesn't depend on it. This introduces the problem that during getInlineCost we cannot return early even if we already know the cost is definitely higher than the threshold. It is a general problem for compile time.

For now, we simply remove ORE check when we initialize ComputeFullInlineCost. That will solve the compile time problem. Although it will miss some extra information in the optimization remark report, currently we don't have a clean way to check whether optimization remark are enabled for either inline or inline-cost passes. If the missing information is a problem, explicitly adding -mllvm -inline-cost-full will solve that.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Feb 19 2019, 11:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 11:26 AM
chandlerc added inline comments.Feb 19 2019, 12:13 PM
lib/Analysis/InlineCost.cpp
282–285 ↗(On Diff #187414)

Can't we query ORE as well as this bool in inline cost?

wmi marked an inline comment as done.Feb 19 2019, 12:35 PM
wmi added inline comments.
lib/Analysis/InlineCost.cpp
282–285 ↗(On Diff #187414)

What we need is an API to check whether optimization remark report for some DEBUG_TYPE will be emitted. When doing the check in inline cost, we still need such API. The API doesn't depend on ORE, but will be implemented using things like OptimizationRemarkMissed::isEnabled. A difficulty here is that DEBUG_TYPE could be inline-cost, or inline. For both DEBUG_TYPE, we want the API to return true, but the check will be put in lib/Analysis/InlineCost.cpp and we only see DEBUG_TYPE to be inline-cost.

One option is to call DiagnosticHandler::isMissedOptRemarkEnabled in Inliner and pass the result (bool) when creating the CallAnalyzer object. This can be ORed with whether the missed opt remark is enabled for inline-cost.

wmi added a comment.Feb 19 2019, 3:20 PM

One option is to call DiagnosticHandler::isMissedOptRemarkEnabled in Inliner and pass the result (bool) when creating the CallAnalyzer object. This can be ORed with whether the missed opt remark is enabled for inline-cost.

That is feasible. Actually I found we don't need to pass the extra bool. By checking the locations where getInlineCost is called, some of them already set RemarksEnabled and passes RemarksEnabled ? &ORE : nullptr to getInlineCost as its ORE argument. I did the same for inliner and partial inliner, so the assumption that "RemarksEnabled == (ORE != nullptr))" can be hold true, and we can still use the same way to set ComputeFullInlineCost.

wmi updated this revision to Diff 187456.Feb 19 2019, 3:21 PM

Address Easwaran and Chandler's comments.

chandlerc added inline comments.Feb 19 2019, 3:31 PM
lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

you can just set ORE to nullptr here?

wmi marked an inline comment as done.Feb 19 2019, 3:43 PM
wmi added inline comments.
lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

I don't get the idea. Could you explain it? Setting ORE to nullptr here then it won't compute the full inline cost even if -Rpass=inline* is used.

wmi marked an inline comment as done.Feb 19 2019, 4:18 PM
wmi added inline comments.
lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

Easwaran discussed with me offline. I think he has the same suggestion with you -- pass RemarksEnabled as another parameter of getInlineCost and set ORE here, so as to avoid creating another Emitter inside of getInlineCost. I will update the patch accordingly.

wmi updated this revision to Diff 187468.Feb 19 2019, 4:26 PM

Address Easwaran and Chandler's comments.

chandlerc added inline comments.Feb 19 2019, 4:34 PM
lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

I do like this better, and my suggestion wasn't very clear as I misunderstood your first response. Sorry about that.

I wonder if we should go one step further: I think the idea that we look at both DEBUG_TYPEs is actually a bad thing. I'd much rather we *only* look at the pass's DEBUG_TYPE (personally). And it has the side-effect of simplifying this somewhat confusing structure here. Thoughts?

wmi marked an inline comment as done.Feb 19 2019, 6:38 PM
wmi added inline comments.
lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

That makes sense to me. Easwaran also suggested eventually optimization remarks emitted in InlineCost.cpp is better to be moved outside too -- return the related information to the caller and let the passes decide whether to emit the remarks. But that deserves a separate change. For now, I just simply remove the remark enabling check for inline-cost DEBUG_TYPE.

wmi updated this revision to Diff 187490.Feb 19 2019, 6:39 PM

Address Chandler's comment.

wmi updated this revision to Diff 187491.Feb 19 2019, 6:42 PM

Add a TODO comment.

chandlerc accepted this revision.Feb 19 2019, 6:43 PM

LGTM (with a minor cleanup, but feel free to just submit w/ that if we're all agreeing about this direction).

lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

nit: You can probably just have the callers pass in a nullptr similar to your previous patch if they're the only ones doing this.

This revision is now accepted and ready to land.Feb 19 2019, 6:43 PM
wmi marked an inline comment as done.Feb 20 2019, 6:45 PM

Thanks for the review!

lib/Analysis/InlineCost.cpp
2008–2012 ↗(On Diff #187456)

Ok, Done

This revision was automatically updated to reflect the committed changes.