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

[wmi]

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.

[chandlerc]

lib/Analysis/InlineCost.cpp

282–285 ↗

(On Diff #187414)

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

[wmi]

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.

[eraman]

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]

In D58399#1402844, @eraman wrote:

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]

Address Easwaran and Chandler's comments.

[chandlerc]

lib/Analysis/InlineCost.cpp

2008–2012 ↗

(On Diff #187456)

you can just set ORE to nullptr here?

[wmi]

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]

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]

Address Easwaran and Chandler's comments.

[chandlerc]

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]

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]

Address Chandler's comment.

[wmi]

Add a TODO comment.

[chandlerc]

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.

[wmi]

Thanks for the review!

lib/Analysis/InlineCost.cpp

2008–2012 ↗

(On Diff #187456)

Ok, Done

llvm/trunk/lib/Transforms/IPO/Inliner.cpp

for (int i = 0; i < (int)Calls.size(); ++i) {
     };
     auto GetBFI = [&](Function &F) -> BlockFrequencyInfo & {
       return FAM.getResult<BlockFrequencyAnalysis>(F);
     };
 
     auto GetInlineCost = [&](CallSite CS) {
       Function &Callee = *CS.getCalledFunction();
       auto &CalleeTTI = FAM.getResult<TargetIRAnalysis>(Callee);
+      bool RemarksEnabled =
+          Callee.getContext().getDiagHandlerPtr()->isMissedOptRemarkEnabled(
+              DEBUG_TYPE);
       return getInlineCost(CS, Params, CalleeTTI, GetAssumptionCache, {GetBFI},
-                           PSI, &ORE);
+                           PSI, RemarksEnabled ? &ORE : nullptr);
     };
 
     // Now process as many calls as we have within this caller in the sequnece.
     // We bail out as soon as the caller has to change so we can update the
     // call graph and prepare the context of that new caller.
     bool DidInline = false;
     for (; i < (int)Calls.size() && Calls[i].first.getCaller() == &F; ++i) {
       int InlineHistoryID;

llvm/trunk/lib/Transforms/IPO/PartialInlining.cpp

bool PartialInlinerImpl::shouldPartialInline(
   Function *Callee = CS.getCalledFunction();
   assert(Callee == Cloner.ClonedFunc);
 
   if (SkipCostAnalysis)
     return isInlineViable(*Callee);
 
   Function *Caller = CS.getCaller();
   auto &CalleeTTI = (*GetTTI)(*Callee);
-  InlineCost IC = getInlineCost(CS, getInlineParams(), CalleeTTI,
-                                *GetAssumptionCache, GetBFI, PSI, &ORE);
+  bool RemarksEnabled =
+      Callee->getContext().getDiagHandlerPtr()->isMissedOptRemarkEnabled(
+          DEBUG_TYPE);
+  InlineCost IC =
+      getInlineCost(CS, getInlineParams(), CalleeTTI, *GetAssumptionCache,
+                    GetBFI, PSI, RemarksEnabled ? &ORE : nullptr);
 
   if (IC.isAlways()) {
     ORE.emit([&]() {
       return OptimizationRemarkAnalysis(DEBUG_TYPE, "AlwaysInline", Call)
              << NV("Callee", Cloner.OrigFunc)
              << " should always be fully inlined, not partially";
     });
     return false;