[llvm] [BOLT] Refactor interface for instruction labels. NFCI (PR #83209)
Maksim Panchenko via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 27 15:44:52 PST 2024
https://github.com/maksfb created https://github.com/llvm/llvm-project/pull/83209
To avoid accidentally setting the label twice for the same instruction, which can lead to a "lost" label, introduce getOrSetInstLabel() function. Rename existing functions to getInstLabel()/setInstLabel() to make it explicit that they operate on instruction labels. Add an assertion in setInstLabel() that the instruction did not have a prior label set.
>From 4566b02fafde0f1e102877dbe8c4e56589bdfec8 Mon Sep 17 00:00:00 2001
From: Maksim Panchenko <maks at fb.com>
Date: Tue, 27 Feb 2024 15:34:55 -0800
Subject: [PATCH] [BOLT] Refactor interface for instruction labels. NFCI
To avoid accidentally setting the label twice for the same instruction,
which can lead to a "lost" label, introduce getOrSetInstLabel()
function. Rename existing functions to getInstLabel()/setInstLabel() to
make it explicit that they operate on instruction labels. Add an
assertion in setInstLabel() that the instruction did not have a prior
label set.
---
bolt/include/bolt/Core/MCPlusBuilder.h | 9 +++++++--
bolt/lib/Core/BinaryContext.cpp | 2 +-
bolt/lib/Core/BinaryEmitter.cpp | 2 +-
bolt/lib/Core/BinaryFunction.cpp | 2 +-
bolt/lib/Core/Exceptions.cpp | 5 ++---
bolt/lib/Core/MCPlusBuilder.cpp | 19 ++++++++++++++++---
bolt/lib/Rewrite/LinuxKernelRewriter.cpp | 14 ++++----------
7 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index eeb7609ff6b5f1..6bb76d1b917db3 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -1183,11 +1183,16 @@ class MCPlusBuilder {
bool clearOffset(MCInst &Inst) const;
/// Return the label of \p Inst, if available.
- MCSymbol *getLabel(const MCInst &Inst) const;
+ MCSymbol *getInstLabel(const MCInst &Inst) const;
+
+ /// Set the label of \p Inst or return the existing label for the instruction.
+ /// This label will be emitted right before \p Inst is emitted to MCStreamer.
+ MCSymbol *getOrCreateInstLabel(MCInst &Inst, const Twine &Name,
+ MCContext *Ctx) const;
/// Set the label of \p Inst. This label will be emitted right before \p Inst
/// is emitted to MCStreamer.
- bool setLabel(MCInst &Inst, MCSymbol *Label) const;
+ void setInstLabel(MCInst &Inst, MCSymbol *Label) const;
/// Get instruction size specified via annotation.
std::optional<uint32_t> getSize(const MCInst &Inst) const;
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index d544ece13a832f..b29ebbbfa18c4b 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -1967,7 +1967,7 @@ void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction,
OS << " # Offset: " << *Offset;
if (std::optional<uint32_t> Size = MIB->getSize(Instruction))
OS << " # Size: " << *Size;
- if (MCSymbol *Label = MIB->getLabel(Instruction))
+ if (MCSymbol *Label = MIB->getInstLabel(Instruction))
OS << " # Label: " << *Label;
MIB->printAnnotations(Instruction, OS);
diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp
index d4b668c1d7e7bd..97d19b75200f51 100644
--- a/bolt/lib/Core/BinaryEmitter.cpp
+++ b/bolt/lib/Core/BinaryEmitter.cpp
@@ -489,7 +489,7 @@ void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, FunctionFragment &FF,
if (!EmitCodeOnly) {
// A symbol to be emitted before the instruction to mark its location.
- MCSymbol *InstrLabel = BC.MIB->getLabel(Instr);
+ MCSymbol *InstrLabel = BC.MIB->getInstLabel(Instr);
if (opts::UpdateDebugSections && BF.getDWARFUnit()) {
LastLocSeen = emitLineInfo(BF, Instr.getLoc(), LastLocSeen,
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 54f2f9d972a461..00df42c11e2239 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1424,7 +1424,7 @@ Error BinaryFunction::disassemble() {
InstrMapType::iterator II = Instructions.find(Offset);
assert(II != Instructions.end() && "reference to non-existing instruction");
- BC.MIB->setLabel(II->second, Label);
+ BC.MIB->setInstLabel(II->second, Label);
}
// Reset symbolizer for the disassembler.
diff --git a/bolt/lib/Core/Exceptions.cpp b/bolt/lib/Core/Exceptions.cpp
index 54618aeb95cccb..82bddf76d5b8c9 100644
--- a/bolt/lib/Core/Exceptions.cpp
+++ b/bolt/lib/Core/Exceptions.cpp
@@ -408,12 +408,11 @@ void BinaryFunction::updateEHRanges() {
// Same symbol is used for the beginning and the end of the range.
MCSymbol *EHSymbol;
- if (MCSymbol *InstrLabel = BC.MIB->getLabel(Instr)) {
+ if (MCSymbol *InstrLabel = BC.MIB->getInstLabel(Instr)) {
EHSymbol = InstrLabel;
} else {
std::unique_lock<llvm::sys::RWMutex> Lock(BC.CtxMutex);
- EHSymbol = BC.Ctx->createNamedTempSymbol("EH");
- BC.MIB->setLabel(Instr, EHSymbol);
+ EHSymbol = BC.MIB->getOrCreateInstLabel(Instr, "EH", BC.Ctx.get());
}
// At this point we could be in one of the following states:
diff --git a/bolt/lib/Core/MCPlusBuilder.cpp b/bolt/lib/Core/MCPlusBuilder.cpp
index 44e5f88d8950fc..4ad831c6044a86 100644
--- a/bolt/lib/Core/MCPlusBuilder.cpp
+++ b/bolt/lib/Core/MCPlusBuilder.cpp
@@ -12,6 +12,7 @@
#include "bolt/Core/MCPlusBuilder.h"
#include "bolt/Core/MCPlus.h"
+#include "llvm/MC/MCContext.h"
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstrAnalysis.h"
#include "llvm/MC/MCInstrDesc.h"
@@ -266,17 +267,29 @@ bool MCPlusBuilder::clearOffset(MCInst &Inst) const {
return true;
}
-MCSymbol *MCPlusBuilder::getLabel(const MCInst &Inst) const {
+MCSymbol *MCPlusBuilder::getInstLabel(const MCInst &Inst) const {
if (std::optional<int64_t> Label =
getAnnotationOpValue(Inst, MCAnnotation::kLabel))
return reinterpret_cast<MCSymbol *>(*Label);
return nullptr;
}
-bool MCPlusBuilder::setLabel(MCInst &Inst, MCSymbol *Label) const {
+MCSymbol *MCPlusBuilder::getOrCreateInstLabel(MCInst &Inst, const Twine &Name,
+ MCContext *Ctx) const {
+ MCSymbol *Label = getInstLabel(Inst);
+ if (Label)
+ return Label;
+
+ Label = Ctx->createTempSymbol(Name);
+ setAnnotationOpValue(Inst, MCAnnotation::kLabel,
+ reinterpret_cast<int64_t>(Label));
+ return Label;
+}
+
+void MCPlusBuilder::setInstLabel(MCInst &Inst, MCSymbol *Label) const {
+ assert(!getInstLabel(Inst) && "Instruction already has assigned label.");
setAnnotationOpValue(Inst, MCAnnotation::kLabel,
reinterpret_cast<int64_t>(Label));
- return true;
}
std::optional<uint32_t> MCPlusBuilder::getSize(const MCInst &Inst) const {
diff --git a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
index 6377c1197253c8..0d7dc1070ce75e 100644
--- a/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
+++ b/bolt/lib/Rewrite/LinuxKernelRewriter.cpp
@@ -770,11 +770,8 @@ Error LinuxKernelRewriter::rewriteORCTables() {
continue;
// Issue label for the instruction.
- MCSymbol *Label = BC.MIB->getLabel(Inst);
- if (!Label) {
- Label = BC.Ctx->createTempSymbol("__ORC_");
- BC.MIB->setLabel(Inst, Label);
- }
+ MCSymbol *Label =
+ BC.MIB->getOrCreateInstLabel(Inst, "__ORC_", BC.Ctx.get());
if (Error E = emitORCEntry(0, *ErrorOrState, Label))
return E;
@@ -908,11 +905,8 @@ Error LinuxKernelRewriter::readStaticCalls() {
BC.MIB->addAnnotation(*Inst, "StaticCall", EntryID);
- MCSymbol *Label = BC.MIB->getLabel(*Inst);
- if (!Label) {
- Label = BC.Ctx->createTempSymbol("__SC_");
- BC.MIB->setLabel(*Inst, Label);
- }
+ MCSymbol *Label =
+ BC.MIB->getOrCreateInstLabel(*Inst, "__SC_", BC.Ctx.get());
StaticCallEntries.push_back({EntryID, BF, Label});
}
More information about the llvm-commits
mailing list