[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

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

[wmi]

lib/Analysis/InlineCost.cpp

282–285

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

2010–2014

you can just set ORE to nullptr here?

[wmi]

lib/Analysis/InlineCost.cpp

2010–2014

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

2010–2014

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

2010–2014

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

2010–2014

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

2010–2014

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

2010–2014

Ok, Done

include/llvm/Analysis/InlineCost.h

 /// Note that a default threshold is passed into this function. This threshold
 /// could be modified based on callsite's properties and only costs below this
 /// new threshold are computed with any accuracy. The new threshold can be
 /// used to bound the computation necessary to determine whether the cost is
 /// sufficiently low to warrant inlining.
 ///
 /// Also note that calling this function *dynamically* computes the cost of
 /// inlining the callsite. It is an expensive, heavyweight call.
-InlineCost getInlineCost(
-    CallSite CS, const InlineParams &Params, TargetTransformInfo &CalleeTTI,
+InlineCost
+getInlineCost(CallSite CS, const InlineParams &Params,
+              TargetTransformInfo &CalleeTTI,
               std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
               Optional<function_ref<BlockFrequencyInfo &(Function &)>> GetBFI,
-    ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE = nullptr);
+              ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE = nullptr,
+              bool RemarksEnabled = false);
 
 /// Get an InlineCost with the callee explicitly specified.
 /// This allows you to calculate the cost of inlining a function via a
 /// pointer. This behaves exactly as the version with no explicit callee
 /// parameter in all other respects.
 //
 InlineCost
 getInlineCost(CallSite CS, Function *Callee, const InlineParams &Params,
               TargetTransformInfo &CalleeTTI,
               std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
               Optional<function_ref<BlockFrequencyInfo &(Function &)>> GetBFI,
-              ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE);
+              ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
+              bool RemarksEnabled);
 
 /// Minimal filter to detect invalid constructs for inlining.
 bool isInlineViable(Function &Callee);
 }
 
 #endif

lib/Analysis/InlineCost.cpp

public:
   CallAnalyzer(const TargetTransformInfo &TTI,
                std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
                Optional<function_ref<BlockFrequencyInfo &(Function &)>> &GetBFI,
                ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
                Function &Callee, CallSite CSArg, const InlineParams &Params)
       : TTI(TTI), GetAssumptionCache(GetAssumptionCache), GetBFI(GetBFI),
         PSI(PSI), F(Callee), DL(F.getParent()->getDataLayout()), ORE(ORE),
         CandidateCS(CSArg), Params(Params), Threshold(Params.DefaultThreshold),
         Cost(0), ComputeFullInlineCost(OptComputeFullInlineCost ||
                                        Params.ComputeFullInlineCost || ORE),
         IsCallerRecursive(false), IsRecursiveCall(false),
         ExposesReturnsTwice(false), HasDynamicAlloca(false),
         ContainsNoDuplicateCall(false), HasReturn(false), HasIndirectBr(false),
         HasUninlineableIntrinsic(false), InitsVargArgs(false), AllocatedSize(0),
         NumInstructions(0), NumVectorInstructions(0), VectorBonus(0),
         SingleBBBonus(0), EnableLoadElimination(true), LoadEliminationCost(0),
         NumConstantArgs(0), NumConstantOffsetPtrArgs(0), NumAllocaArgs(0),
         NumConstantPtrCmps(0), NumConstantPtrDiffs(0),
         NumInstructionsSimplified(0), SROACostSavings(0),
         SROACostSavingsLost(0) {}
int llvm::getCallsiteCost(CallSite CS, const DataLayout &DL) {
   Cost += InlineConstants::InstrCost + InlineConstants::CallPenalty;
   return Cost;
 }
 
 InlineCost llvm::getInlineCost(
     CallSite CS, const InlineParams &Params, TargetTransformInfo &CalleeTTI,
     std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
     Optional<function_ref<BlockFrequencyInfo &(Function &)>> GetBFI,
-    ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE) {
+    ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
+    bool RemarksEnabled) {
   return getInlineCost(CS, CS.getCalledFunction(), Params, CalleeTTI,
-                       GetAssumptionCache, GetBFI, PSI, ORE);
+                       GetAssumptionCache, GetBFI, PSI, ORE, RemarksEnabled);
 }
 
 InlineCost llvm::getInlineCost(
     CallSite CS, Function *Callee, const InlineParams &Params,
     TargetTransformInfo &CalleeTTI,
     std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
     Optional<function_ref<BlockFrequencyInfo &(Function &)>> GetBFI,
-    ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE) {
+    ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE,
+    bool RemarksEnabled) {
 
   // Cannot inline indirect calls.
   if (!Callee)
     return llvm::InlineCost::getNever("indirect call");
 
+  // TODO: Eventually optimization remarks emitted in InlineCost.cpp is better
+  // to be moved outside -- return the related information to the caller and
+  // let the passes decide whether to emit the remarks.
+  if (!RemarksEnabled)
+    ORE = nullptr;
+
   // Never inline calls with byval arguments that does not have the alloca
   // address space. Since byval arguments can be replaced with a copy to an
   // alloca, the inlined code would need to be adjusted to handle that the
   // argument is in the alloca address space (so it is a little bit complicated
   // to solve).
   unsigned AllocaAS = Callee->getParent()->getDataLayout().getAllocaAddrSpace();
   for (unsigned I = 0, E = CS.arg_size(); I != E; ++I)
     if (CS.isByValArgument(I)) {

lib/Target/AMDGPU/AMDGPUInline.cpp

InlineCost AMDGPUInliner::getInlineCost(CallSite CS) {
 
   OptimizationRemarkEmitter ORE(Caller);
   std::function<AssumptionCache &(Function &)> GetAssumptionCache =
       [this](Function &F) -> AssumptionCache & {
     return ACT->getAssumptionCache(F);
   };
 
   return llvm::getInlineCost(CS, Callee, LocalParams, TTI, GetAssumptionCache,
-                             None, PSI, RemarksEnabled ? &ORE : nullptr);
+                             None, PSI, &ORE, RemarksEnabled);
 }

lib/Transforms/IPO/InlineSimple.cpp

InlineCost getInlineCost(CallSite CS) override {
     }
     OptimizationRemarkEmitter ORE(CS.getCaller());
 
     std::function<AssumptionCache &(Function &)> GetAssumptionCache =
         [&](Function &F) -> AssumptionCache & {
       return ACT->getAssumptionCache(F);
     };
     return llvm::getInlineCost(CS, Params, TTI, GetAssumptionCache,
-                               /*GetBFI=*/None, PSI,
-                               RemarksEnabled ? &ORE : nullptr);
+                               /*GetBFI=*/None, PSI, &ORE, RemarksEnabled);
   }
 
   bool runOnSCC(CallGraphSCC &SCC) override;
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
 private:
   TargetTransformInfoWrapperPass *TTIWP;
 

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, &ORE, RemarksEnabled);
     };
 
     // 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;

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, &ORE, RemarksEnabled);
 
   if (IC.isAlways()) {
     ORE.emit([&]() {
       return OptimizationRemarkAnalysis(DEBUG_TYPE, "AlwaysInline", Call)
              << NV("Callee", Cloner.OrigFunc)
              << " should always be fully inlined, not partially";
     });
     return false;