Skip to content

Commit

Permalink
[BOLT] Fix hfsort+ caching mechanism
Browse files Browse the repository at this point in the history
Summary:
There's good news and bad news.

The good news is that this fixes the caching mechanism used by hfsort+ so that we always get the correct end results, i.e. the order is the same whether the cache is enabled or not.
The bad news is that it takes about the same amount of time as the original to run. (~6min)
The good news is that I can make some improvements on this implementation which I'll put up in another diff.

The problem with the old caching mechanism is that it was caching values that were dependent on adjacent sets of clusters.  It only invalidated the clusters being merged and none of other clusters that might have been affected.  This version computes the adjacency information up front and updates it after every merge, rather than recomputing it for each iteration.  It uses the adjacency data to properly invalidate any cached values.

(cherry picked from FBD5203023)
  • Loading branch information
Bill Nell authored and maksfb committed Jun 7, 2017
1 parent 583790e commit ea53066
Show file tree
Hide file tree
Showing 7 changed files with 335 additions and 160 deletions.
22 changes: 17 additions & 5 deletions bolt/Passes/BinaryFunctionCallGraph.cpp
Expand Up @@ -12,13 +12,14 @@
#include "BinaryFunctionCallGraph.h"
#include "BinaryFunction.h"
#include "BinaryContext.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Options.h"
#include "llvm/Support/Timer.h"

#define DEBUG_TYPE "callgraph"

namespace opts {
extern llvm::cl::opt<bool> TimeOpts;
extern llvm::cl::opt<unsigned> Verbosity;
}

namespace llvm {
Expand Down Expand Up @@ -130,8 +131,11 @@ BinaryFunctionCallGraph buildCallGraph(BinaryContext &BC,
const auto DstId = lookupNode(DstFunc);
const auto AvgDelta = !UseEdgeCounts ? Offset - DstFunc->getAddress() : 0;
Cg.incArcWeight(SrcId, DstId, Count, AvgDelta);
DEBUG(dbgs() << "BOLT-DEBUG: buildCallGraph: call " << *Function
<< " -> " << *DstFunc << " @ " << Offset << "\n");
DEBUG(
if (opts::Verbosity > 1) {
dbgs() << "BOLT-DEBUG: buildCallGraph: call " << *Function
<< " -> " << *DstFunc << " @ " << Offset << "\n";
});
return true;
}
return false;
Expand Down Expand Up @@ -194,8 +198,16 @@ BinaryFunctionCallGraph buildCallGraph(BinaryContext &BC,
}
}

outs() << "BOLT-WARNING: buildCallGraph: " << NotProcessed
<< " callsites not processed out of " << TotalCalls << "\n";
#ifndef NDEBUG
bool PrintInfo = DebugFlag && isCurrentDebugType("callgraph");
#else
bool PrintInfo = false;
#endif
if (PrintInfo || opts::Verbosity > 0) {
outs() << format("BOLT-INFO: buildCallGraph: %u nodes, density = %.6lf, "
"%u callsites not processed out of %u.\n",
Cg.numNodes(), Cg.density(), NotProcessed, TotalCalls);
}

return Cg;
}
Expand Down
2 changes: 0 additions & 2 deletions bolt/Passes/CallGraph.cpp
Expand Up @@ -10,8 +10,6 @@
//===----------------------------------------------------------------------===//

#include "CallGraph.h"
#include "BinaryFunction.h"
#include "BinaryContext.h"

#define DEBUG_TYPE "callgraph"

Expand Down
4 changes: 4 additions & 0 deletions bolt/Passes/CallGraph.h
Expand Up @@ -130,6 +130,10 @@ class CallGraph {
return Arcs;
}

double density() const {
return double(Arcs.size()) / (Nodes.size()*Nodes.size());
}

void normalizeArcWeights(bool UseEdgeCounts);

template <typename L>
Expand Down
18 changes: 12 additions & 6 deletions bolt/Passes/HFSort.cpp
Expand Up @@ -112,17 +112,22 @@ void Cluster::reverseTargets() {
std::reverse(Targets.begin(), Targets.end());
}

void Cluster::merge(Cluster&& Other, const double Aw) {
void Cluster::merge(const Cluster& Other, const double Aw) {
Targets.insert(Targets.end(),
Other.Targets.begin(),
Other.Targets.end());
Size += Other.Size;
Samples += Other.Samples;
Density = (double)Samples / Size;
}

Other.Size = 0;
Other.Samples = 0;
Other.Targets.clear();
void Cluster::clear() {
Id = -1u;
Size = 0;
Samples = 0;
Density = 0.0;
Targets.clear();
Frozen = false;
}

std::vector<Cluster> clusterize(const CallGraph &Cg) {
Expand Down Expand Up @@ -218,7 +223,8 @@ std::vector<Cluster> clusterize(const CallGraph &Cg) {
FuncCluster[F] = PredCluster;
}

PredCluster->merge(std::move(*Cluster));
PredCluster->merge(*Cluster);
Cluster->clear();
}

// Return the set of Clusters that are left, which are the ones that
Expand Down Expand Up @@ -281,7 +287,7 @@ std::vector<Cluster> randomClusters(const CallGraph &Cg) {
if (MergeIdx == Clusters.size()) {
++Idx;
} else {
Clusters[Idx].merge(std::move(Clusters[MergeIdx]));
Clusters[Idx].merge(Clusters[MergeIdx]);
Clusters.erase(Clusters.begin() + MergeIdx);
}
}
Expand Down
8 changes: 5 additions & 3 deletions bolt/Passes/HFSort.h
Expand Up @@ -55,7 +55,8 @@ class Cluster {
uint32_t size() const { return Size; }
bool frozen() const { return Frozen; }
void freeze() { Frozen = true; }
void merge(Cluster &&Other, const double Aw = 0);
void merge(const Cluster &Other, const double Aw = 0);
void clear();
size_t numTargets() const {
return Targets.size();
}
Expand All @@ -66,12 +67,13 @@ class Cluster {
return Targets[N];
}
void reverseTargets();
bool hasId() const { return Id != -1u; }
void setId(uint32_t NewId) {
assert(Id == -1u);
assert(!hasId());
Id = NewId;
}
uint32_t id() const {
assert(Id != -1u);
assert(hasId());
return Id;
}
private:
Expand Down

0 comments on commit ea53066

Please sign in to comment.