[llvm][NFC] Inliner: factor cost and reporting out of inlining process

[mtrofin]

Summary

This factors cost and reporting out of the inlining workflow, thus
making it easier to reuse when driving inlining from the upcoming
InliningAdvisor.

Depends on: D79215

[dblaikie]

llvm/lib/Transforms/IPO/Inliner.cpp

514

If you don't need to mutate it, you can pass DebugLoc by value (quick grep across llvm/include shows that to be quite common) - it's small/cheap to copy.

& changes like this can just be committed for post-commit review

(ah, I see what motivated you to fix/change this - your new/changed callsite, that passes a const& DebugLoc... *goes over there to comment*)

1058

Can /probably skip the "Callee" in these enumerators, since because they're in an enum class, they'll always be qualified by "CalleeStatus" (so "CalleeStatus::Deleted" and "CalleeStatus::Kept" is probably readable, I think? (could even rename the enum to just "Callee" if that helps, since it's scoped inside InliningOutcome, so it's got some context helping it along from there too))

1060

Looks like at least in this change, only the "Result" is used? Probably defer adding the CalleeStatus until you're introducing a use of it, to help justify it, ensure it's tested/exercised, etc?

1062

Would it be practical for this to be a standalone function, or does it use too much state from the outer function? As it stands, this, InlinerPass::run is already quite long - pulling any chunks with clearly describable goals out into separate functions (entirely separate, rather than nested like this) might make it easier to read the high level algorithm at a glance, rather than spread over many pages?

1143–1169

In a follow-up patch (no need for review) this could be refactored to reduce indentation by using early-returns ( https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code ). eg:

if (!Callee.hasLocalLinkage)
  return {InliningOutcome::CalleeStatus::Kept, IR};

Callee.removeDeadConstantUsers();

if (!Callee.use_empty() || CG.isLibFunction(Callee))
  return {InliningOutcome::CalleeStatus::Kept, IR};

Calls.erase(...);
...
return {InliningOutcome::CalleeStatus::Deleted, IR};

1174

Best to just copy (rather than reference) here - same as if you had a function that returned "int" by value.

[davidxl]

so there will be a follow up refactoring to move the DoInline lamda into a standalone helper?

[mtrofin]

llvm/lib/Transforms/IPO/Inliner.cpp

1058

Ack - will incorporate when we need it - removed the type for now. Left comment as "undone" for tracking.

1062

Yes, that's my intention. This first patch just keeps the changes to a minimum. Next, I'll peel out the stuff that's bound to the context, but can be executed after the function (which turns out to be all of it, I verified). Then we can hoist the code out in its own function.

1143–1169

Ack, let's do that once this is factored out as a function. Leaving this comment as "undone", for easy tracking.

[mtrofin]

In D79275#2016617, @davidxl wrote:

so there will be a follow up refactoring to move the DoInline lamda into a standalone helper?

That's my intention, yes.

[mtrofin]

feedback

[dblaikie]

Looks good to me (I'd probably have pulled it out into a separate function in one go (as it stands the code feels awkwardly complicated - creating a lambda for just one caller - I generally try to make the code self-consistent/justified with each commit, insofar as that's possible) but given you're headed that way, whichever way you find best to get there seems OK)

llvm/lib/Transforms/IPO/Inliner.cpp

bool LegacyInlinerBase::runOnSCC(CallGraphSCC &SCC) {
   if (skipSCC(SCC))
     return false;
   return inlineCalls(SCC);
 }
 
 static void emitInlinedInto(OptimizationRemarkEmitter &ORE, DebugLoc &DLoc,
                             const BasicBlock *Block, const Function &Callee,
                             const Function &Caller, const InlineCost &IC) {
   ORE.emit([&]() {
     bool AlwaysInline = IC.isAlways();
     StringRef RemarkName = AlwaysInline ? "AlwaysInline" : "Inlined";
     return OptimizationRemark(DEBUG_TYPE, RemarkName, DLoc, Block)
            << ore::NV("Callee", &Callee) << " inlined into "
            << ore::NV("Caller", &Caller) << " with " << IC;
   });
 }
 
for (; I < (int)Calls.size() && Calls[I].first->getCaller() == &F; ++I) {
         setInlineRemark(*CB, "recursive SCC split");
         continue;
       }
 
       auto OIC = shouldInline(*CB, GetInlineCost, ORE);
       // Check whether we want to inline this callsite.
       if (!OIC)
         continue;
-
+      auto DoInline = [&]() -> InlineResult {
         // Setup the data structure used to plumb customization into the
         // `InlineFunction` routine.
         InlineFunctionInfo IFI(
             /*cg=*/nullptr, &GetAssumptionCache, PSI,
             &FAM.getResult<BlockFrequencyAnalysis>(*(CB->getCaller())),
             &FAM.getResult<BlockFrequencyAnalysis>(Callee));
 
-      // Get DebugLoc to report. CB will be invalid after Inliner.
-      DebugLoc DLoc = CB->getDebugLoc();
-      BasicBlock *Block = CB->getParent();
-
-      using namespace ore;
-
         InlineResult IR = InlineFunction(*CB, IFI);
-      if (!IR.isSuccess()) {
-        setInlineRemark(*CB, std::string(IR.getFailureReason()) + "; " +
-                                 inlineCostStr(*OIC));
-        ORE.emit([&]() {
-          return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc, Block)
-                 << NV("Callee", &Callee) << " will not be inlined into "
-                 << NV("Caller", &F) << ": "
-                 << NV("Reason", IR.getFailureReason());
-        });
-        continue;
-      }
+        if (!IR.isSuccess())
+          return IR;
+
         DidInline = true;
         InlinedCallees.insert(&Callee);
-
         ++NumInlined;
 
-      emitInlinedInto(ORE, DLoc, Block, Callee, F, *OIC);
-
         // Add any new callsites to defined functions to the worklist.
         if (!IFI.InlinedCallSites.empty()) {
           int NewHistoryID = InlineHistory.size();
           InlineHistory.push_back({&Callee, InlineHistoryID});
 
           for (CallBase *ICB : reverse(IFI.InlinedCallSites)) {
             Function *NewCallee = ICB->getCalledFunction();
             if (!NewCallee) {
-            // Try to promote an indirect (virtual) call without waiting for the
-            // post-inline cleanup and the next DevirtSCCRepeatedPass iteration
-            // because the next iteration may not happen and we may miss
-            // inlining it.
+              // Try to promote an indirect (virtual) call without waiting for
+              // the post-inline cleanup and the next DevirtSCCRepeatedPass
+              // iteration because the next iteration may not happen and we may
+              // miss inlining it.
               if (tryPromoteCall(*ICB))
                 NewCallee = ICB->getCalledFunction();
             }
             if (NewCallee)
               if (!NewCallee->isDeclaration())
                 Calls.push_back({ICB, NewHistoryID});
           }
         }
 
         if (InlinerFunctionImportStats != InlinerFunctionImportStatsOpts::No)
           ImportedFunctionsStats->recordInline(F, Callee);
 
         // Merge the attributes based on the inlining.
         AttributeFuncs::mergeAttributesForInlining(F, Callee);
 
         // For local functions, check whether this makes the callee trivially
         // dead. In that case, we can drop the body of the function eagerly
         // which may reduce the number of callers of other functions to one,
         // changing inline cost thresholds.
         if (Callee.hasLocalLinkage()) {
           // To check this we also need to nuke any dead constant uses (perhaps
           // made dead by this operation on other functions).
           Callee.removeDeadConstantUsers();
           if (Callee.use_empty() && !CG.isLibFunction(Callee)) {
             Calls.erase(
                 std::remove_if(Calls.begin() + I + 1, Calls.end(),
                                [&](const std::pair<CallBase *, int> &Call) {
                                  return Call.first->getCaller() == &Callee;
                                }),
                 Calls.end());
             // Clear the body and queue the function itself for deletion when we
             // finish inlining and call graph updates.
             // Note that after this point, it is an error to do anything other
             // than use the callee's address or delete it.
             Callee.dropAllReferences();
             assert(find(DeadFunctions, &Callee) == DeadFunctions.end() &&
                    "Cannot put cause a function to become dead twice!");
             DeadFunctions.push_back(&Callee);
           }
         }
+        return IR;
+      };
+      // Capture the context of CB before inlining, as a successful inlining may
+      // change that context, and we want to report success or failure in the
+      // original context.
+      auto DLoc = CB->getDebugLoc();
+      auto *Block = CB->getParent();
+
+      auto Outcome = DoInline();
+      if (!Outcome.isSuccess()) {
+        using namespace ore;
+        setInlineRemark(*CB, std::string(Outcome.getFailureReason()) + "; " +
+                                 inlineCostStr(*OIC));
+        ORE.emit([&]() {
+          return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc, Block)
+                 << NV("Callee", &Callee) << " will not be inlined into "
+                 << NV("Caller", &F) << ": "
+                 << NV("Reason", Outcome.getFailureReason());
+        });
+        continue;
+      }
+
+      emitInlinedInto(ORE, DLoc, Block, Callee, F, *OIC);
     }
 
     // Back the call index up by one to put us in a good position to go around
     // the outer loop.
     --I;
 
     if (!DidInline)
       continue;
     Changed = true;
 
     // Add all the inlined callees' edges as ref edges to the caller. These are
     // by definition trivial edges as we always have *some* transitive ref edge
     // chain. While in some cases these edges are direct calls inside the
     // callee, they have to be modeled in the inliner as reference edges as
     // there may be a reference edge anywhere along the chain from the current
     // caller to the callee that causes the whole thing to appear like
     // a (transitive) reference edge that will require promotion to a call edge
     // below.
     for (Function *InlinedCallee : InlinedCallees) {
       LazyCallGraph::Node &CalleeN = *CG.lookup(*InlinedCallee);
       for (LazyCallGraph::Edge &E : *CalleeN)
         RC->insertTrivialRefEdge(N, E.getNode());
     }
 
     // At this point, since we have made changes we have at least removed
     // a call instruction. However, in the process we do some incremental
     // simplification of the surrounding code. This simplification can
     // essentially do all of the same things as a function pass and we can
     // re-use the exact same logic for updating the call graph to reflect the
     // change.
     LazyCallGraph::SCC *OldC = C;
     C = &updateCGAndAnalysisManagerForFunctionPass(CG, *C, N, AM, UR);
     LLVM_DEBUG(dbgs() << "Updated inlining SCC: " << *C << "\n");
     RC = &C->getOuterRefSCC();