Skip to content

[BOLT][NFC] Track fragment relationships using EquivalenceClasses #99979

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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 14 additions & 1 deletion bolt/include/bolt/Core/BinaryContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "bolt/RuntimeLibs/RuntimeLibrary.h"
#include "llvm/ADT/AddressRanges.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/EquivalenceClasses.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/iterator.h"
#include "llvm/BinaryFormat/Dwarf.h"
Expand Down Expand Up @@ -241,6 +242,10 @@ class BinaryContext {
/// Function fragments to skip.
std::unordered_set<BinaryFunction *> FragmentsToSkip;

/// Fragment equivalence classes to query belonging to the same "family" in
/// presence of multiple fragments/multiple parents.
EquivalenceClasses<const BinaryFunction *> FragmentClasses;
Copy link
Contributor

Choose a reason for hiding this comment

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

How expensive is this data structure, considering we can work with 1M+ functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a set that only keeps a copy of entry (a pointer in our case):

/// EquivalenceClasses - This represents a collection of equivalence classes and
/// supports three efficient operations: insert an element into a class of its
/// own, union two classes, and find the class for a given element. In
/// addition to these modification methods, it is possible to iterate over all
/// of the equivalence classes and all of the elements in a class.
///
/// This implementation is an efficient implementation that only stores one copy
/// of the element being indexed per entry in the set, and allows any arbitrary
/// type to be indexed (as long as it can be ordered with operator< or a
/// comparator is provided).

Moreover, we will only insert fragments into it, so it shouldn't be too expensive even for the largest binaries (we split up to 10s of thousands of functions).


/// The runtime library.
std::unique_ptr<RuntimeLibrary> RtLibrary;

Expand Down Expand Up @@ -1032,7 +1037,15 @@ class BinaryContext {
/// fragment_name == parent_name.cold(.\d+)?
/// True if the Function is registered, false if the check failed.
bool registerFragment(BinaryFunction &TargetFunction,
BinaryFunction &Function) const;
BinaryFunction &Function);

/// Return true if two functions belong to the same "family": are fragments
/// of one another, or fragments of the same parent, or transitively fragment-
/// related.
bool areRelatedFragments(const BinaryFunction *LHS,
const BinaryFunction *RHS) const {
return FragmentClasses.isEquivalent(LHS, RHS);
}

/// Add interprocedural reference for \p Function to \p Address
void addInterproceduralReference(BinaryFunction *Function, uint64_t Address) {
Expand Down
10 changes: 0 additions & 10 deletions bolt/include/bolt/Core/BinaryFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1793,11 +1793,6 @@ class BinaryFunction {
return ParentFragments.contains(&Other);
}

/// Returns if this function is a parent of \p Other function.
bool isParentOf(const BinaryFunction &Other) const {
return Fragments.contains(&Other);
}

/// Return the child fragment form parent function
iterator_range<FragmentsSetTy::const_iterator> getFragments() const {
return iterator_range<FragmentsSetTy::const_iterator>(Fragments.begin(),
Expand All @@ -1807,11 +1802,6 @@ class BinaryFunction {
/// Return the parent function for split function fragments.
FragmentsSetTy *getParentFragments() { return &ParentFragments; }

/// Returns if this function is a parent or child of \p Other function.
bool isParentOrChildOf(const BinaryFunction &Other) const {
return isChildOf(Other) || isParentOf(Other);
}

/// Set the profile data for the number of times the function was called.
BinaryFunction &setExecutionCount(uint64_t Count) {
ExecutionCount = Count;
Expand Down
9 changes: 5 additions & 4 deletions bolt/lib/Core/BinaryContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address,
const BinaryFunction *TargetBF = getBinaryFunctionContainingAddress(Value);
const bool DoesBelongToFunction =
BF.containsAddress(Value) ||
(TargetBF && TargetBF->isParentOrChildOf(BF));
(TargetBF && areRelatedFragments(TargetBF, &BF));
if (!DoesBelongToFunction) {
LLVM_DEBUG({
if (!BF.containsAddress(Value)) {
Expand Down Expand Up @@ -841,7 +841,7 @@ BinaryContext::getOrCreateJumpTable(BinaryFunction &Function, uint64_t Address,
// Prevent associating a jump table to a specific fragment twice.
// This simple check arises from the assumption: no more than 2 fragments.
if (JT->Parents.size() == 1 && JT->Parents[0] != &Function) {
assert(JT->Parents[0]->isParentOrChildOf(Function) &&
assert(areRelatedFragments(JT->Parents[0], &Function) &&
"cannot re-use jump table of a different function");
// Duplicate the entry for the parent function for easy access
JT->Parents.push_back(&Function);
Expand Down Expand Up @@ -1209,12 +1209,13 @@ void BinaryContext::generateSymbolHashes() {
}

bool BinaryContext::registerFragment(BinaryFunction &TargetFunction,
BinaryFunction &Function) const {
BinaryFunction &Function) {
assert(TargetFunction.isFragment() && "TargetFunction must be a fragment");
if (TargetFunction.isChildOf(Function))
return true;
TargetFunction.addParentFragment(Function);
Function.addFragment(TargetFunction);
FragmentClasses.unionSets(&TargetFunction, &Function);
if (!HasRelocations) {
TargetFunction.setSimple(false);
Function.setSimple(false);
Expand Down Expand Up @@ -1336,7 +1337,7 @@ void BinaryContext::processInterproceduralReferences() {

if (TargetFunction) {
if (TargetFunction->isFragment() &&
!TargetFunction->isChildOf(Function)) {
!areRelatedFragments(TargetFunction, &Function)) {
this->errs()
<< "BOLT-WARNING: interprocedural reference between unrelated "
"fragments: "
Expand Down
2 changes: 1 addition & 1 deletion bolt/lib/Core/Exceptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ Error BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
"BOLT-ERROR: cannot find landing pad fragment");
BC.addInterproceduralReference(this, Fragment->getAddress());
BC.processInterproceduralReferences();
assert(isParentOrChildOf(*Fragment) &&
assert(BC.areRelatedFragments(this, Fragment) &&
"BOLT-ERROR: cannot have landing pads in different functions");
setHasIndirectTargetToSplitFragment(true);
BC.addFragmentsToSkip(this);
Expand Down
Loading