Skip to content

Commit 587d265

Browse files
author
Fabian Parzefall
committed
[BOLT] Towards FunctionLayout const-correctness
A const-qualified reference to function layout allows accessing non-const qualified basic blocks on a const-qualified function. This patch adds or removes const-qualifiers where necessary to indicate where basic blocks are used in a non-const manner. Reviewed By: rafauler Differential Revision: https://reviews.llvm.org/D132049
1 parent 13cb085 commit 587d265

17 files changed

+87
-71
lines changed

‎bolt/include/bolt/Core/BinaryBasicBlock.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,9 @@ class BinaryBasicBlock {
424424
/// Return branch info corresponding to an edge going to \p Succ basic block.
425425
BinaryBranchInfo &getBranchInfo(const BinaryBasicBlock &Succ);
426426

427+
/// Return branch info corresponding to an edge going to \p Succ basic block.
428+
const BinaryBranchInfo &getBranchInfo(const BinaryBasicBlock &Succ) const;
429+
427430
/// Return branch info corresponding to an edge going to a basic block with
428431
/// label \p Label.
429432
BinaryBranchInfo &getBranchInfo(const MCSymbol *Label);

‎bolt/include/bolt/Core/BinaryFunction.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1599,7 +1599,7 @@ class BinaryFunction {
15991599

16001600
/// Print function information to the \p OS stream.
16011601
void print(raw_ostream &OS, std::string Annotation = "",
1602-
bool PrintInstructions = true) const;
1602+
bool PrintInstructions = true);
16031603

16041604
/// Print all relocations between \p Offset and \p Offset + \p Size in
16051605
/// this function.

‎bolt/include/bolt/Core/DynoStats.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,11 @@ DynoStats operator+(const DynoStats &A, const DynoStats &B);
142142
/// The function relies on branch instructions being in-sync with CFG for
143143
/// branch instructions stats. Thus it is better to call it after
144144
/// fixBranches().
145-
DynoStats getDynoStats(const BinaryFunction &BF);
145+
DynoStats getDynoStats(BinaryFunction &BF);
146146

147147
/// Return program-wide dynostats.
148148
template <typename FuncsType>
149-
inline DynoStats getDynoStats(const FuncsType &Funcs, bool IsAArch64) {
149+
inline DynoStats getDynoStats(FuncsType &Funcs, bool IsAArch64) {
150150
DynoStats dynoStats(IsAArch64);
151151
for (auto &BFI : Funcs) {
152152
auto &BF = BFI.second;
@@ -158,9 +158,8 @@ inline DynoStats getDynoStats(const FuncsType &Funcs, bool IsAArch64) {
158158

159159
/// Call a function with optional before and after dynostats printing.
160160
template <typename FnType, typename FuncsType>
161-
inline void callWithDynoStats(FnType &&Func, const FuncsType &Funcs,
162-
StringRef Phase, const bool Flag,
163-
bool IsAArch64) {
161+
inline void callWithDynoStats(FnType &&Func, FuncsType &Funcs, StringRef Phase,
162+
const bool Flag, bool IsAArch64) {
164163
DynoStats DynoStatsBefore(IsAArch64);
165164
if (Flag)
166165
DynoStatsBefore = getDynoStats(Funcs, IsAArch64);

‎bolt/include/bolt/Passes/ReorderAlgorithm.h

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class ClusterAlgorithm {
3939
/// Clusters vector. Also encode relative weights between two clusters in
4040
/// the ClusterEdges vector if requested. This vector is indexed by
4141
/// the clusters indices in the Clusters vector.
42-
virtual void clusterBasicBlocks(const BinaryFunction &BF,
42+
virtual void clusterBasicBlocks(BinaryFunction &BF,
4343
bool ComputeEdges = false) = 0;
4444

4545
/// Compute for each cluster its averagae execution frequency, that is
@@ -98,7 +98,7 @@ class GreedyClusterAlgorithm : public ClusterAlgorithm {
9898
BBToClusterMapTy BBToClusterMap;
9999

100100
public:
101-
void clusterBasicBlocks(const BinaryFunction &BF,
101+
void clusterBasicBlocks(BinaryFunction &BF,
102102
bool ComputeEdges = false) override;
103103
void reset() override;
104104
};
@@ -167,7 +167,7 @@ class ReorderAlgorithm {
167167

168168
/// Reorder the basic blocks of the given function and store the new order in
169169
/// the new Clusters vector.
170-
virtual void reorderBasicBlocks(const BinaryFunction &BF,
170+
virtual void reorderBasicBlocks(BinaryFunction &BF,
171171
BasicBlockOrder &Order) const = 0;
172172

173173
void setClusterAlgorithm(ClusterAlgorithm *CAlgo) {
@@ -185,7 +185,7 @@ class ReorderAlgorithm {
185185
/// only be used for small functions.
186186
class TSPReorderAlgorithm : public ReorderAlgorithm {
187187
public:
188-
void reorderBasicBlocks(const BinaryFunction &BF,
188+
void reorderBasicBlocks(BinaryFunction &BF,
189189
BasicBlockOrder &Order) const override;
190190
};
191191

@@ -196,7 +196,7 @@ class OptimizeReorderAlgorithm : public ReorderAlgorithm {
196196
explicit OptimizeReorderAlgorithm(std::unique_ptr<ClusterAlgorithm> CAlgo)
197197
: ReorderAlgorithm(std::move(CAlgo)) {}
198198

199-
void reorderBasicBlocks(const BinaryFunction &BF,
199+
void reorderBasicBlocks(BinaryFunction &BF,
200200
BasicBlockOrder &Order) const override;
201201
};
202202

@@ -209,7 +209,7 @@ class OptimizeBranchReorderAlgorithm : public ReorderAlgorithm {
209209
std::unique_ptr<ClusterAlgorithm> CAlgo)
210210
: ReorderAlgorithm(std::move(CAlgo)) {}
211211

212-
void reorderBasicBlocks(const BinaryFunction &BF,
212+
void reorderBasicBlocks(BinaryFunction &BF,
213213
BasicBlockOrder &Order) const override;
214214
};
215215

@@ -222,21 +222,21 @@ class OptimizeCacheReorderAlgorithm : public ReorderAlgorithm {
222222
std::unique_ptr<ClusterAlgorithm> CAlgo)
223223
: ReorderAlgorithm(std::move(CAlgo)) {}
224224

225-
void reorderBasicBlocks(const BinaryFunction &BF,
225+
void reorderBasicBlocks(BinaryFunction &BF,
226226
BasicBlockOrder &Order) const override;
227227
};
228228

229229
/// A new reordering algorithm for basic blocks, ext-tsp
230230
class ExtTSPReorderAlgorithm : public ReorderAlgorithm {
231231
public:
232-
void reorderBasicBlocks(const BinaryFunction &BF,
232+
void reorderBasicBlocks(BinaryFunction &BF,
233233
BasicBlockOrder &Order) const override;
234234
};
235235

236236
/// Toy example that simply reverses the original basic block order.
237237
class ReverseReorderAlgorithm : public ReorderAlgorithm {
238238
public:
239-
void reorderBasicBlocks(const BinaryFunction &BF,
239+
void reorderBasicBlocks(BinaryFunction &BF,
240240
BasicBlockOrder &Order) const override;
241241
};
242242

@@ -247,7 +247,7 @@ class RandomClusterReorderAlgorithm : public ReorderAlgorithm {
247247
std::unique_ptr<ClusterAlgorithm> CAlgo)
248248
: ReorderAlgorithm(std::move(CAlgo)) {}
249249

250-
void reorderBasicBlocks(const BinaryFunction &BF,
250+
void reorderBasicBlocks(BinaryFunction &BF,
251251
BasicBlockOrder &Order) const override;
252252
};
253253

‎bolt/lib/Core/BinaryBasicBlock.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -581,15 +581,17 @@ uint64_t BinaryBasicBlock::estimateSize(const MCCodeEmitter *Emitter) const {
581581

582582
BinaryBasicBlock::BinaryBranchInfo &
583583
BinaryBasicBlock::getBranchInfo(const BinaryBasicBlock &Succ) {
584-
auto BI = branch_info_begin();
585-
for (BinaryBasicBlock *BB : successors()) {
586-
if (&Succ == BB)
587-
return *BI;
588-
++BI;
589-
}
584+
return const_cast<BinaryBranchInfo &>(
585+
static_cast<const BinaryBasicBlock &>(*this).getBranchInfo(Succ));
586+
}
590587

591-
llvm_unreachable("Invalid successor");
592-
return *BI;
588+
const BinaryBasicBlock::BinaryBranchInfo &
589+
BinaryBasicBlock::getBranchInfo(const BinaryBasicBlock &Succ) const {
590+
const auto Zip = llvm::zip(successors(), branch_info());
591+
const auto Result = llvm::find_if(
592+
Zip, [&](const auto &Tuple) { return std::get<0>(Tuple) == &Succ; });
593+
assert(Result != Zip.end() && "Cannot find target in successors");
594+
return std::get<1>(*Result);
593595
}
594596

595597
BinaryBasicBlock::BinaryBranchInfo &

‎bolt/lib/Core/BinaryFunction.cpp

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -396,11 +396,20 @@ bool BinaryFunction::isForwardCall(const MCSymbol *CalleeSymbol) const {
396396
}
397397

398398
void BinaryFunction::dump(bool PrintInstructions) const {
399-
print(dbgs(), "", PrintInstructions);
399+
// getDynoStats calls FunctionLayout::updateLayoutIndices and
400+
// BasicBlock::analyzeBranch. The former cannot be const, but should be
401+
// removed, the latter should be made const, but seems to require refactoring.
402+
// Forcing all callers to have a non-const reference to BinaryFunction to call
403+
// dump non-const however is not ideal either. Adding this const_cast is right
404+
// now the best solution. It is safe, because BinaryFunction itself is not
405+
// modified. Only BinaryBasicBlocks are actually modified (if it all) and we
406+
// have mutable pointers to those regardless whether this function is
407+
// const-qualified or not.
408+
const_cast<BinaryFunction &>(*this).print(dbgs(), "", PrintInstructions);
400409
}
401410

402411
void BinaryFunction::print(raw_ostream &OS, std::string Annotation,
403-
bool PrintInstructions) const {
412+
bool PrintInstructions) {
404413
if (!opts::shouldPrint(*this))
405414
return;
406415

@@ -3558,9 +3567,9 @@ size_t BinaryFunction::computeHash(bool UseDFS,
35583567

35593568
assert(hasCFG() && "function is expected to have CFG");
35603569

3561-
BasicBlockListType Order;
3570+
SmallVector<const BinaryBasicBlock *, 0> Order;
35623571
if (UseDFS)
3563-
Order = dfs();
3572+
llvm::copy(dfs(), std::back_inserter(Order));
35643573
else
35653574
llvm::copy(Layout.blocks(), std::back_inserter(Order));
35663575

‎bolt/lib/Core/DynoStats.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ void DynoStats::operator+=(const DynoStats &Other) {
158158
}
159159
}
160160

161-
DynoStats getDynoStats(const BinaryFunction &BF) {
161+
DynoStats getDynoStats(BinaryFunction &BF) {
162162
auto &BC = BF.getBinaryContext();
163163

164164
DynoStats Stats(/*PrintAArch64Stats*/ BC.isAArch64());

‎bolt/lib/Passes/AsmDump.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ void dumpFunction(const BinaryFunction &BF) {
195195
std::unordered_set<const MCSymbol *> CallReferences;
196196

197197
MAP->OutStreamer->emitCFIStartProc(/*IsSimple=*/false);
198-
for (BinaryBasicBlock *BB : BF.getLayout().blocks()) {
198+
for (const BinaryBasicBlock *BB : BF.getLayout().blocks()) {
199199
OS << BB->getName() << ": \n";
200200

201201
const std::string BranchLabel = Twine(BB->getName(), "_br").str();

‎bolt/lib/Passes/BinaryPasses.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1450,11 +1450,11 @@ void PrintProgramStats::runOnFunctions(BinaryContext &BC) {
14501450
if (!opts::PrintSortedBy.empty() &&
14511451
!llvm::is_contained(opts::PrintSortedBy, DynoStats::FIRST_DYNO_STAT)) {
14521452

1453-
std::vector<const BinaryFunction *> Functions;
1453+
std::vector<BinaryFunction *> Functions;
14541454
std::map<const BinaryFunction *, DynoStats> Stats;
14551455

1456-
for (const auto &BFI : BC.getBinaryFunctions()) {
1457-
const BinaryFunction &BF = BFI.second;
1456+
for (auto &BFI : BC.getBinaryFunctions()) {
1457+
BinaryFunction &BF = BFI.second;
14581458
if (shouldOptimize(BF) && BF.hasValidProfile()) {
14591459
Functions.push_back(&BF);
14601460
Stats.emplace(&BF, getDynoStats(BF));
@@ -1554,9 +1554,9 @@ void PrintProgramStats::runOnFunctions(BinaryContext &BC) {
15541554

15551555
// Collect and print information about suboptimal code layout on input.
15561556
if (opts::ReportBadLayout) {
1557-
std::vector<const BinaryFunction *> SuboptimalFuncs;
1557+
std::vector<BinaryFunction *> SuboptimalFuncs;
15581558
for (auto &BFI : BC.getBinaryFunctions()) {
1559-
const BinaryFunction &BF = BFI.second;
1559+
BinaryFunction &BF = BFI.second;
15601560
if (!BF.hasValidProfile())
15611561
continue;
15621562

‎bolt/lib/Passes/CacheMetrics.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,9 @@ extractFunctionCalls(const std::vector<BinaryFunction *> &BinaryFunctions) {
117117

118118
for (BinaryFunction *SrcFunction : BinaryFunctions) {
119119
const BinaryContext &BC = SrcFunction->getBinaryContext();
120-
for (BinaryBasicBlock *BB : SrcFunction->getLayout().blocks()) {
120+
for (const BinaryBasicBlock *BB : SrcFunction->getLayout().blocks()) {
121121
// Find call instructions and extract target symbols from each one
122-
for (MCInst &Inst : *BB) {
122+
for (const MCInst &Inst : *BB) {
123123
if (!BC.MIB->isCall(Inst))
124124
continue;
125125

‎bolt/lib/Passes/ExtTSPReorderAlgorithm.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ bool compareChainPairs(const Chain *A1, const Chain *B1, const Chain *A2,
441441
}
442442
class ExtTSP {
443443
public:
444-
ExtTSP(const BinaryFunction &BF) : BF(BF) { initialize(); }
444+
ExtTSP(BinaryFunction &BF) : BF(BF) { initialize(); }
445445

446446
/// Run the algorithm and return an ordering of basic block
447447
void run(BinaryFunction::BasicBlockOrderType &Order) {
@@ -849,7 +849,7 @@ class ExtTSP {
849849

850850
private:
851851
// The binary function
852-
const BinaryFunction &BF;
852+
BinaryFunction &BF;
853853

854854
// All CFG nodes (basic blocks)
855855
std::vector<Block> AllBlocks;
@@ -864,7 +864,7 @@ class ExtTSP {
864864
std::vector<Edge> AllEdges;
865865
};
866866

867-
void ExtTSPReorderAlgorithm::reorderBasicBlocks(const BinaryFunction &BF,
867+
void ExtTSPReorderAlgorithm::reorderBasicBlocks(BinaryFunction &BF,
868868
BasicBlockOrder &Order) const {
869869
if (BF.getLayout().block_empty())
870870
return;

‎bolt/lib/Passes/IdenticalCodeFolding.cpp

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,12 @@
1212

1313
#include "bolt/Passes/IdenticalCodeFolding.h"
1414
#include "bolt/Core/ParallelUtilities.h"
15+
#include "llvm/ADT/SmallVector.h"
1516
#include "llvm/Support/CommandLine.h"
1617
#include "llvm/Support/ThreadPool.h"
1718
#include "llvm/Support/Timer.h"
1819
#include <atomic>
20+
#include <iterator>
1921
#include <map>
2022
#include <set>
2123
#include <unordered_map>
@@ -166,16 +168,15 @@ bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
166168
return false;
167169

168170
// Process both functions in either DFS or existing order.
169-
const BinaryFunction::BasicBlockOrderType OrderA =
170-
opts::UseDFS
171-
? A.dfs()
172-
: BinaryFunction::BasicBlockOrderType(A.getLayout().block_begin(),
173-
A.getLayout().block_end());
174-
const BinaryFunction::BasicBlockOrderType OrderB =
175-
opts::UseDFS
176-
? B.dfs()
177-
: BinaryFunction::BasicBlockOrderType(B.getLayout().block_begin(),
178-
B.getLayout().block_end());
171+
SmallVector<const BinaryBasicBlock *, 0> OrderA;
172+
SmallVector<const BinaryBasicBlock *, 0> OrderB;
173+
if (opts::UseDFS) {
174+
copy(A.dfs(), std::back_inserter(OrderA));
175+
copy(B.dfs(), std::back_inserter(OrderB));
176+
} else {
177+
copy(A.getLayout().blocks(), std::back_inserter(OrderA));
178+
copy(B.getLayout().blocks(), std::back_inserter(OrderB));
179+
}
179180

180181
const BinaryContext &BC = A.getBinaryContext();
181182

0 commit comments

Comments
 (0)