[llvm] 48ca6da - Revert "[CSSPGO] Factor out common part for CSSPGO inline and AFDO inline"
Adrian Kuegel via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 4 01:15:05 PST 2021
Or at least it was my intention to comment on that change. Looking back, it
seems my comment never made it, I guess I didn't press some button or
something? I am not very familiar with Phabricator.
I hope it didn't cause too much trouble to figure out why the revert was
needed? At least all the bots with asserts enabled should have been
complaining :)
On Thu, Feb 4, 2021 at 9:54 AM Adrian Kuegel <akuegel at google.com> wrote:
> Sorry, forgot to do this in the commit message of the reverts. But I
> commented on the change I reverted that the tests are failing with asserts
> enabled.
>
> On Wed, Feb 3, 2021 at 8:01 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> 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 <(214)%20748-3647>'
>> > +;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 <(214)%20748-3647>)
>> 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 <(214)%20748-3647>'
>> > +;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
>>
>
>
--
Google Germany GmbH
Erika-Mann-Straße 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210204/eacb45ac/attachment.html>
More information about the llvm-commits
mailing list