Skip to content

Commit a9cd49d

Browse files
aaupovaaupov
committed
[BOLT][NFC] Move Offset annotation to Group 1
Summary: Move the annotation to avoid dynamic memory allocations. Improves the CPU time of instrumenting a large binary by 1% (+-0.8%, p-value 0.01) Test Plan: NFC Reviewers: maksfb FBD30091656
1 parent 4777eb2 commit a9cd49d

14 files changed

+69
-34
lines changed

‎bolt/include/bolt/Core/MCPlus.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ class MCAnnotation {
6565
kJumpTable, /// Jump Table.
6666
kTailCall, /// Tail call.
6767
kConditionalTailCall, /// CTC.
68+
kOffset, /// Offset in the function.
6869
kGeneric /// First generic annotation.
6970
};
7071

‎bolt/include/bolt/Core/MCPlusBuilder.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,18 @@ class MCPlusBuilder {
10751075
/// branch. Return true if the instruction was converted.
10761076
bool unsetConditionalTailCall(MCInst &Inst);
10771077

1078+
/// Return offset of \p Inst in the original function, if available.
1079+
Optional<uint32_t> getOffset(const MCInst &Inst) const;
1080+
1081+
/// Return the offset if the annotation is present, or \p Default otherwise.
1082+
uint32_t getOffsetWithDefault(const MCInst &Inst, uint32_t Default) const;
1083+
1084+
/// Set offset of \p Inst in the original function.
1085+
bool setOffset(MCInst &Inst, uint32_t Offset, AllocatorIdTy AllocatorId = 0);
1086+
1087+
/// Remove offset annotation.
1088+
bool clearOffset(MCInst &Inst);
1089+
10781090
/// Return MCSymbol that represents a target of this instruction at a given
10791091
/// operand number \p OpNum. If there's no symbol associated with
10801092
/// the operand - return nullptr.

‎bolt/lib/Core/BinaryContext.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1632,6 +1632,8 @@ void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction,
16321632
OS << " # UNKNOWN CONTROL FLOW";
16331633
}
16341634
}
1635+
if (Optional<uint32_t> Offset = MIB->getOffset(Instruction))
1636+
OS << " # Offset: " << *Offset;
16351637

16361638
MIB->printAnnotations(Instruction, OS);
16371639

‎bolt/lib/Core/BinaryEmitter.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -468,8 +468,8 @@ void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, bool EmitColdPart,
468468
// Prepare to tag this location with a label if we need to keep track of
469469
// the location of calls/returns for BOLT address translation maps
470470
if (!EmitCodeOnly && BF.requiresAddressTranslation() &&
471-
BC.MIB->hasAnnotation(Instr, "Offset")) {
472-
const auto Offset = BC.MIB->getAnnotationAs<uint32_t>(Instr, "Offset");
471+
BC.MIB->getOffset(Instr)) {
472+
const uint32_t Offset = *BC.MIB->getOffset(Instr);
473473
MCSymbol *LocSym = BC.Ctx->createTempSymbol();
474474
Streamer.emitLabel(LocSym);
475475
BB->getLocSyms().emplace_back(Offset, LocSym);

‎bolt/lib/Core/BinaryFunction.cpp

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1406,7 +1406,7 @@ bool BinaryFunction::disassemble() {
14061406

14071407
// Record offset of the instruction for profile matching.
14081408
if (BC.keepOffsetForInstruction(Instruction))
1409-
MIB->addAnnotation(Instruction, "Offset", static_cast<uint32_t>(Offset));
1409+
MIB->setOffset(Instruction, static_cast<uint32_t>(Offset));
14101410

14111411
if (BC.MIB->isNoop(Instruction)) {
14121412
// NOTE: disassembly loses the correct size information for noops.
@@ -1989,9 +1989,8 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
19891989
break;
19901990
}
19911991
}
1992-
if (LastNonNop && !MIB->hasAnnotation(*LastNonNop, "Offset"))
1993-
MIB->addAnnotation(*LastNonNop, "Offset", static_cast<uint32_t>(Offset),
1994-
AllocatorId);
1992+
if (LastNonNop && !MIB->getOffset(*LastNonNop))
1993+
MIB->setOffset(*LastNonNop, static_cast<uint32_t>(Offset), AllocatorId);
19951994
};
19961995

19971996
for (auto I = Instructions.begin(), E = Instructions.end(); I != E; ++I) {
@@ -2014,9 +2013,8 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
20142013
bool IsLKMarker = BC.LKMarkers.count(InstrInputAddr);
20152014
// Mark all nops with Offset for profile tracking purposes.
20162015
if (MIB->isNoop(Instr) || IsLKMarker) {
2017-
if (!MIB->hasAnnotation(Instr, "Offset"))
2018-
MIB->addAnnotation(Instr, "Offset", static_cast<uint32_t>(Offset),
2019-
AllocatorId);
2016+
if (!MIB->getOffset(Instr))
2017+
MIB->setOffset(Instr, static_cast<uint32_t>(Offset), AllocatorId);
20202018
if (IsSDTMarker || IsLKMarker)
20212019
HasSDTMarker = true;
20222020
else
@@ -2216,7 +2214,7 @@ void BinaryFunction::postProcessCFG() {
22162214
if (!requiresAddressTranslation() && !opts::Instrument) {
22172215
for (BinaryBasicBlock *BB : layout())
22182216
for (MCInst &Inst : *BB)
2219-
BC.MIB->removeAnnotation(Inst, "Offset");
2217+
BC.MIB->clearOffset(Inst);
22202218
}
22212219

22222220
assert((!isSimple() || validateCFG()) &&
@@ -2233,8 +2231,7 @@ void BinaryFunction::calculateMacroOpFusionStats() {
22332231

22342232
// Check offset of the second instruction.
22352233
// FIXME: arch-specific.
2236-
const uint32_t Offset =
2237-
BC.MIB->getAnnotationWithDefault<uint32_t>(*std::next(II), "Offset", 0);
2234+
const uint32_t Offset = BC.MIB->getOffsetWithDefault(*std::next(II), 0);
22382235
if (!Offset || (getAddress() + Offset) % 64)
22392236
continue;
22402237

@@ -4325,8 +4322,7 @@ MCInst *BinaryFunction::getInstructionAtOffset(uint64_t Offset) {
43254322

43264323
for (MCInst &Inst : *BB) {
43274324
constexpr uint32_t InvalidOffset = std::numeric_limits<uint32_t>::max();
4328-
if (Offset == BC.MIB->getAnnotationWithDefault<uint32_t>(Inst, "Offset",
4329-
InvalidOffset))
4325+
if (Offset == BC.MIB->getOffsetWithDefault(Inst, InvalidOffset))
43304326
return &Inst;
43314327
}
43324328

‎bolt/lib/Core/MCPlusBuilder.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,33 @@ bool MCPlusBuilder::unsetConditionalTailCall(MCInst &Inst) {
228228
return true;
229229
}
230230

231+
Optional<uint32_t> MCPlusBuilder::getOffset(const MCInst &Inst) const {
232+
Optional<int64_t> Value = getAnnotationOpValue(Inst, MCAnnotation::kOffset);
233+
if (!Value)
234+
return NoneType();
235+
return static_cast<uint32_t>(*Value);
236+
}
237+
238+
uint32_t MCPlusBuilder::getOffsetWithDefault(const MCInst &Inst,
239+
uint32_t Default) const {
240+
if (Optional<uint32_t> Offset = getOffset(Inst))
241+
return *Offset;
242+
return Default;
243+
}
244+
245+
bool MCPlusBuilder::setOffset(MCInst &Inst, uint32_t Offset,
246+
AllocatorIdTy AllocatorId) {
247+
setAnnotationOpValue(Inst, MCAnnotation::kOffset, Offset, AllocatorId);
248+
return true;
249+
}
250+
251+
bool MCPlusBuilder::clearOffset(MCInst &Inst) {
252+
if (!hasAnnotation(Inst, MCAnnotation::kOffset))
253+
return false;
254+
removeAnnotation(Inst, MCAnnotation::kOffset);
255+
return true;
256+
}
257+
231258
bool MCPlusBuilder::hasAnnotation(const MCInst &Inst, unsigned Index) const {
232259
const MCInst *AnnotationInst = getAnnotationInst(Inst);
233260
if (!AnnotationInst)

‎bolt/lib/Passes/BinaryPasses.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -629,10 +629,9 @@ void LowerAnnotations::runOnFunctions(BinaryContext &BC) {
629629

630630
// Now record preserved annotations separately and then strip annotations.
631631
for (auto II = BB->begin(); II != BB->end(); ++II) {
632-
if (BF.requiresAddressTranslation() &&
633-
BC.MIB->hasAnnotation(*II, "Offset"))
634-
PreservedOffsetAnnotations.emplace_back(
635-
&(*II), BC.MIB->getAnnotationAs<uint32_t>(*II, "Offset"));
632+
if (BF.requiresAddressTranslation() && BC.MIB->getOffset(*II))
633+
PreservedOffsetAnnotations.emplace_back(&(*II),
634+
*BC.MIB->getOffset(*II));
636635
BC.MIB->stripAnnotations(*II);
637636
}
638637
}
@@ -647,7 +646,7 @@ void LowerAnnotations::runOnFunctions(BinaryContext &BC) {
647646

648647
// Reinsert preserved annotations we need during code emission.
649648
for (const std::pair<MCInst *, uint32_t> &Item : PreservedOffsetAnnotations)
650-
BC.MIB->addAnnotation<uint32_t>(*Item.first, "Offset", Item.second);
649+
BC.MIB->setOffset(*Item.first, Item.second);
651650
}
652651

653652
namespace {

‎bolt/lib/Passes/Instrumentation.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
377377

378378
for (auto I = BB.begin(); I != BB.end(); ++I) {
379379
const MCInst &Inst = *I;
380-
if (!BC.MIB->hasAnnotation(Inst, "Offset"))
380+
if (!BC.MIB->getOffset(Inst))
381381
continue;
382382

383383
const bool IsJumpTable = Function.getJumpTable(Inst);
@@ -389,7 +389,7 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
389389
BC.MIB->isUnsupportedBranch(Inst.getOpcode()))
390390
continue;
391391

392-
uint32_t FromOffset = BC.MIB->getAnnotationAs<uint32_t>(Inst, "Offset");
392+
const uint32_t FromOffset = *BC.MIB->getOffset(Inst);
393393
const MCSymbol *Target = BC.MIB->getTargetSymbol(Inst);
394394
BinaryBasicBlock *TargetBB = Function.getBasicBlockForLabel(Target);
395395
uint32_t ToOffset = TargetBB ? TargetBB->getInputOffset() : 0;
@@ -465,9 +465,9 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
465465
// if it was branching to the end of the function as a result of
466466
// __builtin_unreachable(), in which case it was deleted by fixBranches.
467467
// Ignore this case. FIXME: force fixBranches() to preserve the offset.
468-
if (!BC.MIB->hasAnnotation(*LastInstr, "Offset"))
468+
if (!BC.MIB->getOffset(*LastInstr))
469469
continue;
470-
FromOffset = BC.MIB->getAnnotationAs<uint32_t>(*LastInstr, "Offset");
470+
FromOffset = *BC.MIB->getOffset(*LastInstr);
471471

472472
// Do not instrument edges in the spanning tree
473473
if (STOutSet[&BB].find(FTBB) != STOutSet[&BB].end()) {

‎bolt/lib/Profile/DataAggregator.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -914,7 +914,7 @@ bool DataAggregator::recordTrace(
914914
const MCInst *Instr = BB->getLastNonPseudoInstr();
915915
uint64_t Offset = 0;
916916
if (Instr)
917-
Offset = BC.MIB->getAnnotationWithDefault<uint32_t>(*Instr, "Offset");
917+
Offset = BC.MIB->getOffsetWithDefault(*Instr, 0);
918918
else
919919
Offset = BB->getOffset();
920920

‎bolt/lib/Profile/DataReader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,7 @@ bool DataReader::recordBranch(BinaryFunction &BF, uint64_t From, uint64_t To,
721721
const MCInst *LastInstr = ToBB->getLastNonPseudoInstr();
722722
if (LastInstr) {
723723
const uint32_t LastInstrOffset =
724-
BC.MIB->getAnnotationWithDefault<uint32_t>(*LastInstr, "Offset");
724+
BC.MIB->getOffsetWithDefault(*LastInstr, 0);
725725

726726
// With old .fdata we are getting FT branches for "jcc,jmp" sequences.
727727
if (To == LastInstrOffset && BC.MIB->isUnconditionalBranch(*LastInstr))

‎bolt/lib/Profile/YAMLProfileWriter.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,10 @@ void convert(const BinaryFunction &BF,
5353
continue;
5454

5555
yaml::bolt::CallSiteInfo CSI;
56-
auto Offset = BC.MIB->tryGetAnnotationAs<uint32_t>(Instr, "Offset");
57-
if (!Offset || Offset.get() < BB->getInputOffset())
56+
Optional<uint32_t> Offset = BC.MIB->getOffset(Instr);
57+
if (!Offset || *Offset < BB->getInputOffset())
5858
continue;
59-
CSI.Offset = Offset.get() - BB->getInputOffset();
59+
CSI.Offset = *Offset - BB->getInputOffset();
6060

6161
if (BC.MIB->isIndirectCall(Instr) || BC.MIB->isIndirectBranch(Instr)) {
6262
auto ICSP = BC.MIB->tryGetAnnotationAs<IndirectCallSiteProfile>(

‎bolt/lib/Rewrite/RewriteInstance.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2771,7 +2771,6 @@ void RewriteInstance::buildFunctionsCFG() {
27712771
"Build Binary Functions", opts::TimeBuild);
27722772

27732773
// Create annotation indices to allow lock-free execution
2774-
BC->MIB->getOrCreateAnnotationIndex("Offset");
27752774
BC->MIB->getOrCreateAnnotationIndex("JTIndexReg");
27762775
BC->MIB->getOrCreateAnnotationIndex("NOP");
27772776
BC->MIB->getOrCreateAnnotationIndex("Size");

‎bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -805,7 +805,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
805805

806806
bool convertTailCallToJmp(MCInst &Inst) override {
807807
removeAnnotation(Inst, MCPlus::MCAnnotation::kTailCall);
808-
removeAnnotation(Inst, "Offset");
808+
clearOffset(Inst);
809809
if (getConditionalTailCall(Inst))
810810
unsetConditionalTailCall(Inst);
811811
return true;

‎bolt/lib/Target/X86/X86MCPlusBuilder.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2020,7 +2020,7 @@ class X86MCPlusBuilder : public MCPlusBuilder {
20202020

20212021
Inst.setOpcode(NewOpcode);
20222022
removeAnnotation(Inst, MCPlus::MCAnnotation::kTailCall);
2023-
removeAnnotation(Inst, "Offset");
2023+
clearOffset(Inst);
20242024
return true;
20252025
}
20262026

@@ -3808,10 +3808,9 @@ class X86MCPlusBuilder : public MCPlusBuilder {
38083808

38093809
if (CallOrJmp.getOpcode() == X86::CALL64r ||
38103810
CallOrJmp.getOpcode() == X86::CALL64pcrel32) {
3811-
if (hasAnnotation(CallInst, "Offset"))
3811+
if (Optional<uint32_t> Offset = getOffset(CallInst))
38123812
// Annotated as duplicated call
3813-
addAnnotation(CallOrJmp, "Offset",
3814-
getAnnotationAs<uint32_t>(CallInst, "Offset"));
3813+
setOffset(CallOrJmp, *Offset);
38153814
}
38163815

38173816
if (isInvoke(CallInst) && !isInvoke(CallOrJmp)) {

0 commit comments

Comments
 (0)