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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:531
+static void emitInlinedInto(OptimizationRemarkEmitter &ORE,
+                            const DebugLoc &DLoc, const BasicBlock *Block,
+                            const Function &Callee, const Function &Caller,
----------------
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*)


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1078
+      struct InliningOutcome {
+        enum class CalleeStatus { CalleeDeleted, CalleeKept };
+        CalleeStatus CalleeStatus;
----------------
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))


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1080
+        CalleeStatus CalleeStatus;
+        InlineResult Result;
+      };
----------------
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?


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


================
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).
----------------
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};
```


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1156
+      // original context.
+      auto &DLoc = CB->getDebugLoc();
+      auto *Block = CB->getParent();
----------------
Best to just copy (rather than reference) here - same as if you had a function that returned "int" by value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79275