Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FunctionAttrs] deduce attr cold on functions if all CG paths call a cold function #101298

Closed
wants to merge 5 commits into from
Closed

[FunctionAttrs] deduce attr cold on functions if all CG paths call a cold function #101298

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffcbfa4
[FunctionAttrs] Add tests for deducing attr `cold` on functions; NFC
goldsteinn Jul 31, 2024
85efc82
[FunctionAttrs] deduce attr `cold` on functions if all CG paths call …
goldsteinn Jul 31, 2024
3df7969
fixup fmt
goldsteinn Jul 31, 2024
7d22a7e
Fix fmt 2
goldsteinn Aug 11, 2024
08d018f
Comment
goldsteinn Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions llvm/lib/Transforms/IPO/FunctionAttrs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ STATISTIC(NumNoUnwind, "Number of functions marked as nounwind");
STATISTIC(NumNoFree, "Number of functions marked as nofree");
STATISTIC(NumWillReturn, "Number of functions marked as willreturn");
STATISTIC(NumNoSync, "Number of functions marked as nosync");
STATISTIC(NumCold, "Number of functions marked as cold");

STATISTIC(NumThinLinkNoRecurse,
"Number of functions marked as norecurse during thinlink");
Expand Down Expand Up @@ -1745,6 +1746,7 @@ static bool canReturn(Function &F) {
return false;
}


goldsteinn marked this conversation as resolved.
Show resolved Hide resolved
// Set the noreturn function attribute if possible.
static void addNoReturnAttrs(const SCCNodeSet &SCCNodes,
SmallSet<Function *, 8> &Changed) {
Expand All @@ -1760,6 +1762,70 @@ static void addNoReturnAttrs(const SCCNodeSet &SCCNodes,
}
}

static bool
allBBPathsGoThroughCold(BasicBlock *BB,
SmallDenseMap<BasicBlock *, bool, 16> &Visited) {
// If BB contains a cold callsite this path through the CG is cold.
if (any_of(*BB, [](Instruction &I) {
if (auto *CB = dyn_cast<CallBase>(&I))
return CB->hasFnAttr(Attribute::Cold);
return false;
Copy link
Contributor

@nikic nikic Aug 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a note that this assumes throwing and diverging code paths are cold, which is why this does not check for guaranteed-to-transfer.

})) {
Visited[BB] = true;
return true;
}

auto Succs = successors(BB);
// We found a path that doesn't go through any cold callsite.
if (Succs.empty())
return false;

// We didn't find a cold callsite in this BB, so check that all successors
// contain a cold callsite (or that their successors do).
// Potential TODO: We could use static branch hints to assume certain
// successor paths are inherently cold, irrespective of if they contain a cold
// callsite.
for (auto *Succ : Succs) {
// Start with false, this is necessary to ensure we don't turn loops into
// cold.
auto R = Visited.try_emplace(Succ, false);
if (!R.second) {
if (R.first->second)
continue;
return false;
}
if (!allBBPathsGoThroughCold(Succ, Visited))
return false;
Visited[Succ] = true;
}

return true;
}

static bool allPathsGoThroughCold(Function &F) {
SmallDenseMap<BasicBlock *, bool, 16> Visited;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be simpler to turn this into a BFS traversal?

SmallVector<BB> WorkList, Visited;
WorkList.push(entryBB);

while(!WorkList.empty()) {
  BB = WorkList.pop();
  if (BB contains cold calls)
     continue;

  if (BB contains return/calls without willreturn)
     return true;

   Visited.insert(BB);
  
   for (auto *SuccBB: successors(BB))
       if (!Visited.contains(SuccBB))
           WorkList.push_back(SuccBB);
}
return false;

Copy link
Contributor Author

@goldsteinn goldsteinn Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats what I originally had, but was running into issues with inf loops. (edit: not LLVM inf loops, but marking inf loops as cold).

Basically we need more information that just "did we visit this", we need "did we visit this and find a cold call site". I don't really know how to do that without depth-first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic calculus I had was we can either be marking:

entry:
  br label %loop
loop:
  br label %loop

as cold or we can miss the case:

entry:
br label %loop
loop:
%c = <some val>
br i1 %c, label %loop, label %done
done:
<cold call>

and think its better to err on under-attribution rather than over.

Copy link
Contributor Author

@goldsteinn goldsteinn Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we could of course handle this with a dom tree, but I don't think its worth the compile time/complexity). I think most functions we will mark end up essentially being error wrappers which don't have very complex CGs.

Visited[&F.front()] = false;
return allBBPathsGoThroughCold(&F.front(), Visited);
}

// Set the cold function attribute if possible.
static void addColdAttrs(const SCCNodeSet &SCCNodes,
SmallSet<Function *, 8> &Changed) {
for (Function *F : SCCNodes) {
if (!F || !F->hasExactDefinition() || F->hasFnAttribute(Attribute::Naked) ||
F->hasFnAttribute(Attribute::Cold) || F->hasFnAttribute(Attribute::Hot))
continue;

// Potential TODO: We could add attribute `cold` on functions with `coldcc`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this todo. It has been implemented in GlobalOpt:

llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp

Lines 1997 to 2013 in 5286656

if (hasChangeableCC(&F, ChangeableCCCache)) {
NumInternalFunc++;
TargetTransformInfo &TTI = GetTTI(F);
// Change the calling convention to coldcc if either stress testing is
// enabled or the target would like to use coldcc on functions which are
// cold at all call sites and the callers contain no other non coldcc
// calls.
if (EnableColdCCStressTest ||
(TTI.useColdCCForColdCall(F) &&
isValidCandidateForColdCC(F, GetBFI, AllCallsCold))) {
ChangeableCCCache.erase(&F);
F.setCallingConv(CallingConv::Cold);
changeCallSitesToColdCC(&F);
Changed = true;
NumColdCC++;
}
}

Copy link
Contributor Author

@goldsteinn goldsteinn Aug 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is the other way around where we are add coldcc to functions. The todo is saying add attribute cold on functions that have coldcc.

if (allPathsGoThroughCold(*F)) {
F->addFnAttr(Attribute::Cold);
++NumCold;
Changed.insert(F);
continue;
}
}
}

static bool functionWillReturn(const Function &F) {
// We can infer and propagate function attributes only when we know that the
// definition we'll get at link time is *exactly* the definition we see now.
Expand Down Expand Up @@ -1853,6 +1919,7 @@ deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter,
addArgumentAttrs(Nodes.SCCNodes, Changed);
inferConvergent(Nodes.SCCNodes, Changed);
addNoReturnAttrs(Nodes.SCCNodes, Changed);
addColdAttrs(Nodes.SCCNodes, Changed);
addWillReturn(Nodes.SCCNodes, Changed);
addNoUndefAttrs(Nodes.SCCNodes, Changed);

Expand Down
Loading
Loading