[PATCH] D79275: [llvm][NFC] Inliner: factor cost and reporting out of inlining process
Mircea Trofin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun May 3 10:05:19 PDT 2020
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
More information about the llvm-commits
mailing list