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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 6 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1078
+      struct InliningOutcome {
+        enum class CalleeStatus { CalleeDeleted, CalleeKept };
+        CalleeStatus CalleeStatus;
----------------
dblaikie wrote:
> 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))
Ack - will incorporate when we need it - removed the type for now. Left comment as "undone" for tracking.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1082
+      };
+      auto DoInline = [&]() -> InliningOutcome {
+        // Setup the data structure used to plumb customization into the
----------------
dblaikie wrote:
> 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?
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.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1125-1151
+        // 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).
----------------
dblaikie wrote:
> 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};
> ```
Ack, let's do that once this is factored out as a function. Leaving this comment as "undone", for easy tracking.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79275/new/

https://reviews.llvm.org/D79275