[llvm] 48ca6da - Revert "[CSSPGO] Factor out common part for CSSPGO inline and AFDO inline"
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 3 11:00:58 PST 2021
Please include information about why a patch is being reverted (eg:
buildbot links and quotes from buildbot logs) in the commit message -
handy for archaeology (seeing that someone else attempted a change and
why it couldn't be done that way) and if someone is wondering if the
failure they're seeing would be addressed by this patch.
On Tue, Feb 2, 2021 at 2:51 AM Adrian Kuegel via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Adrian Kuegel
> Date: 2021-02-02T11:51:04+01:00
> New Revision: 48ca6da9d2f534752f5a5263a6209257149dba7d
>
> URL: https://github.com/llvm/llvm-project/commit/48ca6da9d2f534752f5a5263a6209257149dba7d
> DIFF: https://github.com/llvm/llvm-project/commit/48ca6da9d2f534752f5a5263a6209257149dba7d.diff
>
> LOG: Revert "[CSSPGO] Factor out common part for CSSPGO inline and AFDO inline"
>
> This reverts commit 9a03058d6322edb8abc803ba3e436cc62647d979.
>
> Added:
>
>
> Modified:
> llvm/lib/Transforms/IPO/SampleProfile.cpp
> llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
> llvm/test/Transforms/SampleProfile/remarks.ll
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
> index 7865426dff16..665c4078f3ee 100644
> --- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
> +++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
> @@ -416,18 +416,20 @@ class SampleProfileLoader {
> findIndirectCallFunctionSamples(const Instruction &I, uint64_t &Sum) const;
> mutable DenseMap<const DILocation *, const FunctionSamples *> DILocation2SampleMap;
> const FunctionSamples *findFunctionSamples(const Instruction &I) const;
> - // Attempt to promote indirect call and also inline the promoted call
> - bool tryPromoteAndInlineCandidate(
> - Function &F, InlineCandidate &Candidate, uint64_t &Sum,
> - DenseSet<Instruction *> &PromotedInsns,
> - SmallVector<CallBase *, 8> *InlinedCallSites = nullptr);
> + CallBase *tryPromoteIndirectCall(Function &F, StringRef CalleeName,
> + uint64_t &Sum, uint64_t Count, CallBase *I,
> + const char *&Reason);
> + bool inlineCallInstruction(CallBase &CB,
> + const FunctionSamples *CalleeSamples);
> bool inlineHotFunctions(Function &F,
> DenseSet<GlobalValue::GUID> &InlinedGUIDs);
> + // Helper functions call-site prioritized BFS inliner
> + // Will change the main FDO inliner to be work list based directly in
> + // upstream, then merge this change with that and remove the duplication.
> InlineCost shouldInlineCandidate(InlineCandidate &Candidate);
> bool getInlineCandidate(InlineCandidate *NewCandidate, CallBase *CB);
> - bool
> - tryInlineCandidate(InlineCandidate &Candidate,
> - SmallVector<CallBase *, 8> *InlinedCallSites = nullptr);
> + bool tryInlineCandidate(InlineCandidate &Candidate,
> + SmallVector<CallBase *, 8> &InlinedCallSites);
> bool
> inlineHotFunctionsWithPriority(Function &F,
> DenseSet<GlobalValue::GUID> &InlinedGUIDs);
> @@ -1075,45 +1077,70 @@ SampleProfileLoader::findFunctionSamples(const Instruction &Inst) const {
> return it.first->second;
> }
>
> -/// Attempt to promote indirect call and also inline the promoted call.
> -///
> -/// \param F Caller function.
> -/// \param Candidate ICP and inline candidate.
> -/// \param Sum Sum of target counts for indirect call.
> -/// \param PromotedInsns Map to keep track of indirect call already processed.
> -/// \param Candidate ICP and inline candidate.
> -/// \param InlinedCallSite Output vector for new call sites exposed after
> -/// inlining.
> -bool SampleProfileLoader::tryPromoteAndInlineCandidate(
> - Function &F, InlineCandidate &Candidate, uint64_t &Sum,
> - DenseSet<Instruction *> &PromotedInsns,
> - SmallVector<CallBase *, 8> *InlinedCallSite) {
> - const char *Reason = "Callee function not available";
> +CallBase *
> +SampleProfileLoader::tryPromoteIndirectCall(Function &F, StringRef CalleeName,
> + uint64_t &Sum, uint64_t Count,
> + CallBase *I, const char *&Reason) {
> + Reason = "Callee function not available";
> // R->getValue() != &F is to prevent promoting a recursive call.
> // If it is a recursive call, we do not inline it as it could bloat
> // the code exponentially. There is way to better handle this, e.g.
> // clone the caller first, and inline the cloned caller if it is
> // recursive. As llvm does not inline recursive calls, we will
> // simply ignore it instead of handling it explicitly.
> - auto R = SymbolMap.find(Candidate.CalleeSamples->getFuncName());
> + auto R = SymbolMap.find(CalleeName);
> if (R != SymbolMap.end() && R->getValue() &&
> !R->getValue()->isDeclaration() && R->getValue()->getSubprogram() &&
> R->getValue()->hasFnAttribute("use-sample-profile") &&
> - R->getValue() != &F &&
> - isLegalToPromote(*Candidate.CallInstr, R->getValue(), &Reason)) {
> + R->getValue() != &F && isLegalToPromote(*I, R->getValue(), &Reason)) {
> auto *DI =
> - &pgo::promoteIndirectCall(*Candidate.CallInstr, R->getValue(),
> - Candidate.CallsiteCount, Sum, false, ORE);
> - if (DI) {
> - Sum -= Candidate.CallsiteCount;
> - PromotedInsns.insert(Candidate.CallInstr);
> - Candidate.CallInstr = DI;
> - if (isa<CallInst>(DI) || isa<InvokeInst>(DI))
> - return tryInlineCandidate(Candidate, InlinedCallSite);
> + &pgo::promoteIndirectCall(*I, R->getValue(), Count, Sum, false, ORE);
> + Sum -= Count;
> + return DI;
> + }
> + return nullptr;
> +}
> +
> +bool SampleProfileLoader::inlineCallInstruction(
> + CallBase &CB, const FunctionSamples *CalleeSamples) {
> + if (ExternalInlineAdvisor) {
> + auto Advice = ExternalInlineAdvisor->getAdvice(CB);
> + if (!Advice->isInliningRecommended()) {
> + Advice->recordUnattemptedInlining();
> + return false;
> }
> - } else {
> - LLVM_DEBUG(dbgs() << "\nFailed to promote indirect call to "
> - << CalleeFunctionName << " because " << Reason << "\n");
> + // Dummy record, we don't use it for replay.
> + Advice->recordInlining();
> + }
> +
> + Function *CalledFunction = CB.getCalledFunction();
> + assert(CalledFunction);
> + DebugLoc DLoc = CB.getDebugLoc();
> + BasicBlock *BB = CB.getParent();
> + InlineParams Params = getInlineParams();
> + Params.ComputeFullInlineCost = true;
> + // Checks if there is anything in the reachable portion of the callee at
> + // this callsite that makes this inlining potentially illegal. Need to
> + // set ComputeFullInlineCost, otherwise getInlineCost may return early
> + // when cost exceeds threshold without checking all IRs in the callee.
> + // The acutal cost does not matter because we only checks isNever() to
> + // see if it is legal to inline the callsite.
> + InlineCost Cost =
> + getInlineCost(CB, Params, GetTTI(*CalledFunction), GetAC, GetTLI);
> + if (Cost.isNever()) {
> + ORE->emit(OptimizationRemarkAnalysis(CSINLINE_DEBUG, "InlineFail", DLoc, BB)
> + << "incompatible inlining");
> + return false;
> + }
> + InlineFunctionInfo IFI(nullptr, GetAC);
> + if (InlineFunction(CB, IFI).isSuccess()) {
> + // The call to InlineFunction erases I, so we can't pass it here.
> + emitInlinedInto(*ORE, DLoc, BB, *CalledFunction, *BB->getParent(), Cost,
> + true, CSINLINE_DEBUG);
> + if (ProfileIsCS)
> + ContextTracker->markContextSamplesInlined(CalleeSamples);
> + ++NumCSInlined;
> + return true;
> }
> return false;
> }
> @@ -1179,11 +1206,10 @@ bool SampleProfileLoader::inlineHotFunctions(
> "ProfAccForSymsInList should be false when profile-sample-accurate "
> "is enabled");
>
> - DenseMap<CallBase *, const FunctionSamples *> LocalNotInlinedCallSites;
> + DenseMap<CallBase *, const FunctionSamples *> localNotInlinedCallSites;
> bool Changed = false;
> - bool LocalChanged = true;
> - while (LocalChanged) {
> - LocalChanged = false;
> + while (true) {
> + bool LocalChanged = false;
> SmallVector<CallBase *, 10> CIS;
> for (auto &BB : F) {
> bool Hot = false;
> @@ -1197,7 +1223,7 @@ bool SampleProfileLoader::inlineHotFunctions(
> "GUIDToFuncNameMap has to be populated");
> AllCandidates.push_back(CB);
> if (FS->getEntrySamples() > 0 || ProfileIsCS)
> - LocalNotInlinedCallSites.try_emplace(CB, FS);
> + localNotInlinedCallSites.try_emplace(CB, FS);
> if (callsiteIsHot(FS, PSI))
> Hot = true;
> else if (shouldInlineColdCallee(*CB))
> @@ -1215,11 +1241,6 @@ bool SampleProfileLoader::inlineHotFunctions(
> }
> for (CallBase *I : CIS) {
> Function *CalledFunction = I->getCalledFunction();
> - InlineCandidate Candidate = {I,
> - LocalNotInlinedCallSites.count(I)
> - ? LocalNotInlinedCallSites[I]
> - : nullptr,
> - 0 /* dummy count */};
> // Do not inline recursive calls.
> if (CalledFunction == &F)
> continue;
> @@ -1236,16 +1257,30 @@ bool SampleProfileLoader::inlineHotFunctions(
> if (!callsiteIsHot(FS, PSI))
> continue;
>
> - Candidate = {I, FS, FS->getEntrySamples()};
> - if (tryPromoteAndInlineCandidate(F, Candidate, Sum, PromotedInsns)) {
> - LocalNotInlinedCallSites.erase(I);
> - LocalChanged = true;
> + const char *Reason = nullptr;
> + auto CalleeFunctionName = FS->getFuncName();
> + if (CallBase *DI =
> + tryPromoteIndirectCall(F, CalleeFunctionName, Sum,
> + FS->getEntrySamples(), I, Reason)) {
> + PromotedInsns.insert(I);
> + // If profile mismatches, we should not attempt to inline DI.
> + if ((isa<CallInst>(DI) || isa<InvokeInst>(DI)) &&
> + inlineCallInstruction(cast<CallBase>(*DI), FS)) {
> + localNotInlinedCallSites.erase(I);
> + LocalChanged = true;
> + }
> + } else {
> + LLVM_DEBUG(dbgs()
> + << "\nFailed to promote indirect call to "
> + << CalleeFunctionName << " because " << Reason << "\n");
> }
> }
> } else if (CalledFunction && CalledFunction->getSubprogram() &&
> !CalledFunction->isDeclaration()) {
> - if (tryInlineCandidate(Candidate)) {
> - LocalNotInlinedCallSites.erase(I);
> + if (inlineCallInstruction(*I, localNotInlinedCallSites.count(I)
> + ? localNotInlinedCallSites[I]
> + : nullptr)) {
> + localNotInlinedCallSites.erase(I);
> LocalChanged = true;
> }
> } else if (LTOPhase == ThinOrFullLTOPhase::ThinLTOPreLink) {
> @@ -1253,7 +1288,11 @@ bool SampleProfileLoader::inlineHotFunctions(
> InlinedGUIDs, F.getParent(), PSI->getOrCompHotCountThreshold());
> }
> }
> - Changed |= LocalChanged;
> + if (LocalChanged) {
> + Changed = true;
> + } else {
> + break;
> + }
> }
>
> // For CS profile, profile for not inlined context will be merged when
> @@ -1262,7 +1301,7 @@ bool SampleProfileLoader::inlineHotFunctions(
> return Changed;
>
> // Accumulate not inlined callsite information into notInlinedSamples
> - for (const auto &Pair : LocalNotInlinedCallSites) {
> + for (const auto &Pair : localNotInlinedCallSites) {
> CallBase *I = Pair.getFirst();
> Function *Callee = I->getCalledFunction();
> if (!Callee || Callee->isDeclaration())
> @@ -1308,7 +1347,7 @@ bool SampleProfileLoader::inlineHotFunctions(
> }
>
> bool SampleProfileLoader::tryInlineCandidate(
> - InlineCandidate &Candidate, SmallVector<CallBase *, 8> *InlinedCallSites) {
> + InlineCandidate &Candidate, SmallVector<CallBase *, 8> &InlinedCallSites) {
>
> CallBase &CB = *Candidate.CallInstr;
> Function *CalledFunction = CB.getCalledFunction();
> @@ -1333,11 +1372,9 @@ bool SampleProfileLoader::tryInlineCandidate(
> true, CSINLINE_DEBUG);
>
> // Now populate the list of newly exposed call sites.
> - if (InlinedCallSites) {
> - InlinedCallSites->clear();
> - for (auto &I : IFI.InlinedCallSites)
> - InlinedCallSites->push_back(I);
> - }
> + InlinedCallSites.clear();
> + for (auto &I : IFI.InlinedCallSites)
> + InlinedCallSites.push_back(I);
>
> if (ProfileIsCS)
> ContextTracker->markContextSamplesInlined(Candidate.CalleeSamples);
> @@ -1409,16 +1446,18 @@ SampleProfileLoader::shouldInlineCandidate(InlineCandidate &Candidate) {
> InlineCost Cost = getInlineCost(*Candidate.CallInstr, Callee, Params,
> GetTTI(*Callee), GetAC, GetTLI);
>
> - // Honor always inline and never inline from call analyzer
> - if (Cost.isNever() || Cost.isAlways())
> - return Cost;
> -
> // For old FDO inliner, we inline the call site as long as cost is not
> // "Never". The cost-benefit check is done earlier.
> if (!CallsitePrioritizedInline) {
> - return InlineCost::get(Cost.getCost(), INT_MAX);
> + if (Cost.isNever())
> + return Cost;
> + return InlineCost::getAlways("hot callsite previously inlined");
> }
>
> + // Honor always inline and never inline from call analyzer
> + if (Cost.isNever() || Cost.isAlways())
> + return Cost;
> +
> // Otherwise only use the cost from call analyzer, but overwite threshold with
> // Sample PGO threshold.
> return InlineCost::get(Cost.getCost(), SampleThreshold);
> @@ -1503,23 +1542,34 @@ bool SampleProfileLoader::inlineHotFunctionsWithPriority(
> // fixed, but we generate
> diff erent types).
> if (!PSI->isHotCount(EntryCountDistributed))
> break;
> - SmallVector<CallBase *, 8> InlinedCallSites;
> - // Attach function profile for promoted indirect callee, and update
> - // call site count for the promoted inline candidate too.
> - Candidate = {I, FS, EntryCountDistributed};
> - if (tryPromoteAndInlineCandidate(F, Candidate, Sum, PromotedInsns,
> - &InlinedCallSites)) {
> - for (auto *CB : InlinedCallSites) {
> - if (getInlineCandidate(&NewCandidate, CB))
> - CQueue.emplace(NewCandidate);
> + const char *Reason = nullptr;
> + auto CalleeFunctionName = FS->getFuncName();
> + if (CallBase *DI = tryPromoteIndirectCall(
> + F, CalleeFunctionName, Sum, EntryCountDistributed, I, Reason)) {
> + // Attach function profile for promoted indirect callee, and update
> + // call site count for the promoted inline candidate too.
> + Candidate = {DI, FS, EntryCountDistributed};
> + PromotedInsns.insert(I);
> + SmallVector<CallBase *, 8> InlinedCallSites;
> + // If profile mismatches, we should not attempt to inline DI.
> + if ((isa<CallInst>(DI) || isa<InvokeInst>(DI)) &&
> + tryInlineCandidate(Candidate, InlinedCallSites)) {
> + for (auto *CB : InlinedCallSites) {
> + if (getInlineCandidate(&NewCandidate, CB))
> + CQueue.emplace(NewCandidate);
> + }
> + Changed = true;
> }
> - Changed = true;
> + } else {
> + LLVM_DEBUG(dbgs()
> + << "\nFailed to promote indirect call to "
> + << CalleeFunctionName << " because " << Reason << "\n");
> }
> }
> } else if (CalledFunction && CalledFunction->getSubprogram() &&
> !CalledFunction->isDeclaration()) {
> SmallVector<CallBase *, 8> InlinedCallSites;
> - if (tryInlineCandidate(Candidate, &InlinedCallSites)) {
> + if (tryInlineCandidate(Candidate, InlinedCallSites)) {
> for (auto *CB : InlinedCallSites) {
> if (getInlineCandidate(&NewCandidate, CB))
> CQueue.emplace(NewCandidate);
>
> diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
> index 5359fd4da067..d47359fa0b5f 100644
> --- a/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
> +++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-inline.ll
> @@ -89,7 +89,7 @@ if.end:
> ;YAML-NEXT: - String: '(cost='
> ;YAML-NEXT: - Cost: '15'
> ;YAML-NEXT: - String: ', threshold='
> -;YAML-NEXT: - Threshold: '2147483647'
> +;YAML-NEXT: - Threshold: '225'
> ;YAML-NEXT: - String: ')'
> ;YAML-NEXT: - String: ' at callsite '
> ;YAML-NEXT: - String: foo
>
> diff --git a/llvm/test/Transforms/SampleProfile/remarks.ll b/llvm/test/Transforms/SampleProfile/remarks.ll
> index 46f016433b20..3add1e74abaa 100644
> --- a/llvm/test/Transforms/SampleProfile/remarks.ll
> +++ b/llvm/test/Transforms/SampleProfile/remarks.ll
> @@ -21,7 +21,7 @@
>
> ; We are expecting foo() to be inlined in main() (almost all the cycles are
> ; spent inside foo).
> -; CHECK: remark: remarks.cc:13:21: _Z3foov inlined into main to match profiling context with (cost=130, threshold=2147483647) at callsite main:0:21;
> +; CHECK: remark: remarks.cc:13:21: _Z3foov inlined into main to match profiling context with (cost=130, threshold=225) at callsite main:0:21;
> ; CHECK: remark: remarks.cc:9:19: rand inlined into main to match profiling context with (cost=always): always inline attribute at callsite _Z3foov:6:19 @ main:0:21;
>
> ; The back edge for the loop is the hottest edge in the loop subgraph.
> @@ -47,7 +47,7 @@
> ;YAML-NEXT: - String: '(cost='
> ;YAML-NEXT: - Cost: '130'
> ;YAML-NEXT: - String: ', threshold='
> -;YAML-NEXT: - Threshold: '2147483647'
> +;YAML-NEXT: - Threshold: '225'
> ;YAML-NEXT: - String: ')'
> ;YAML-NEXT: - String: ' at callsite '
> ;YAML-NEXT: - String: main
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list