-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[AsmPrinter] Reduce AsmPrinterHandlers virt. fn calls #96785
Conversation
Currently, an AsmPrinterHandler has several methods that allow to dynamically hook in unwind or debug info emission, e.g. at begin/end of every function or instruction. The class hierarchy and the actually overriden functions are as follows: (SymSz=setSymbolSize, mFE=markFunctionEnd, BBS=BasicBlockSection, FL=Funclet; b=beginX, e=endX) SymSz Mod Fn mFE BBS FL Inst AsmPrinterHandler - - - - - - - ` PseudoProbeHandler - - - - - - - ` WinCFGuard - e e - - - - ` EHStreamer - - - - - - - ` DwarfCFIException - e be - be - - ` ARMException - - be e - - - ` AIXException - - e - - - - ` WinException - e be e - be - ` WasmException - e be - - - - ` DebugHandlerBase - b be - be - be ` BTFDebug - e - - - - b ` CodeViewDebug - be - - - - b ` DWARFDebug yes be - - - - b Doing virtual function calls per instruction is costly and useless when the called function does nothing. This commit performs the following clean-up/improvements: - PseudoProbeHandler is no longer an AsmPrinterHandler -- it used nothing of its functionality to hook in at the possible points. This avoids virtual function calls when a pseudo probe printer is present. - DebugHandlerBase is no longer an AsmPrinterHandler, but a separate base class. DebugHandlerBase is the only remaining "hook" for begin/end instruction and setSymbolSize (only used by DWARFDebug). begin/end for function and basic block sections are never overriden and therefore are no longer virtual. (Originally I intended there to be only one debug handler, but BPF as the only target supports two at the same time: DWARF and BTF.) - AsmPrinterHandler no longer has begin/end instruction and setSymbolSize hooks -- these were only used by DebugHandlerBase. This avoid iterating over handlers in every instruction. - Remove NamedRegionTimer from instruction loop. Checking a global variable for every instruction (and doing an out-of-line function call) is too expensive for a profiling functionality. AsmPrinterHandler Mod Fn mFE BBS FL ` WinCFGuard e e - - - ` EHStreamer - - - - - ` DwarfCFIException e be - be - ` ARMException - be e - - ` AIXException - e - - - ` WinException e be e - be ` WasmException e be - - - SymSz Mod Fn BBS Inst DebugHandlerBase - b be be be ` BTFDebug - e b ` CodeViewDebug - be b ` DWARFDebug yes be b PseudoProbeHandler (no shared methods) This results in a performance improvement, especially in the -O0 -g0 case with unwind information (e.g., JIT baseline).
@llvm/pr-subscribers-debuginfo Author: Alexis Engelke (aengelke) ChangesCurrently, an AsmPrinterHandler has several methods that allow to dynamically hook in unwind or debug info emission, e.g. at begin/end of every function or instruction. The class hierarchy and the actually overriden functions are as follows:
Doing virtual function calls per instruction is costly and useless when the called function does nothing. This commit performs the following clean-up/improvements:
This results in a performance improvement, especially in the -O0 -g0 case with unwind information (e.g., JIT baseline). Full diff: https://github.com/llvm/llvm-project/pull/96785.diff 13 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index 011f8c6534b6a..317798f25d58c 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -40,6 +40,7 @@ class Constant;
class ConstantArray;
class ConstantPtrAuth;
class DataLayout;
+class DebugHandlerBase;
class DIE;
class DIEAbbrev;
class DwarfDebug;
@@ -208,6 +209,9 @@ class AsmPrinter : public MachineFunctionPass {
std::vector<HandlerInfo> Handlers;
size_t NumUserHandlers = 0;
+ /// Debuginfo handler. Protected so that targets can add their own.
+ SmallVector<std::unique_ptr<DebugHandlerBase>, 1> DebugHandlers;
+
StackMaps SM;
private:
@@ -222,7 +226,7 @@ class AsmPrinter : public MachineFunctionPass {
/// A handler that supports pseudo probe emission with embedded inline
/// context.
- PseudoProbeHandler *PP = nullptr;
+ std::unique_ptr<PseudoProbeHandler> PP;
/// CFISection type the module needs i.e. either .eh_frame or .debug_frame.
CFISection ModuleCFISection = CFISection::None;
diff --git a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
index 5c06645f767eb..ed73e618431de 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
@@ -34,10 +34,6 @@ class AsmPrinterHandler {
public:
virtual ~AsmPrinterHandler();
- /// For symbols that have a size designated (e.g. common symbols),
- /// this tracks that size.
- virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) = 0;
-
virtual void beginModule(Module *M) {}
/// Emit all sections that should come after the content.
@@ -72,12 +68,6 @@ class AsmPrinterHandler {
virtual void beginFunclet(const MachineBasicBlock &MBB,
MCSymbol *Sym = nullptr) {}
virtual void endFunclet() {}
-
- /// Process beginning of an instruction.
- virtual void beginInstruction(const MachineInstr *MI) = 0;
-
- /// Process end of an instruction.
- virtual void endInstruction() = 0;
};
} // End of namespace llvm
diff --git a/llvm/include/llvm/CodeGen/DebugHandlerBase.h b/llvm/include/llvm/CodeGen/DebugHandlerBase.h
index af25f2544da71..0436651a9fd48 100644
--- a/llvm/include/llvm/CodeGen/DebugHandlerBase.h
+++ b/llvm/include/llvm/CodeGen/DebugHandlerBase.h
@@ -50,10 +50,14 @@ struct DbgVariableLocation {
/// Base class for debug information backends. Common functionality related to
/// tracking which variables and scopes are alive at a given PC live here.
-class DebugHandlerBase : public AsmPrinterHandler {
+class DebugHandlerBase {
protected:
DebugHandlerBase(AsmPrinter *A);
+public:
+ virtual ~DebugHandlerBase();
+
+protected:
/// Target of debug info emission.
AsmPrinter *Asm = nullptr;
@@ -116,18 +120,22 @@ class DebugHandlerBase : public AsmPrinterHandler {
private:
InstructionOrdering InstOrdering;
- // AsmPrinterHandler overrides.
public:
- void beginModule(Module *M) override;
+ /// For symbols that have a size designated (e.g. common symbols),
+ /// this tracks that size. Only used by DWARF.
+ virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) {};
+
+ virtual void beginModule(Module *M);
+ virtual void endModule() = 0;
- void beginInstruction(const MachineInstr *MI) override;
- void endInstruction() override;
+ virtual void beginInstruction(const MachineInstr *MI);
+ virtual void endInstruction();
- void beginFunction(const MachineFunction *MF) override;
- void endFunction(const MachineFunction *MF) override;
+ void beginFunction(const MachineFunction *MF);
+ void endFunction(const MachineFunction *MF);
- void beginBasicBlockSection(const MachineBasicBlock &MBB) override;
- void endBasicBlockSection(const MachineBasicBlock &MBB) override;
+ void beginBasicBlockSection(const MachineBasicBlock &MBB);
+ void endBasicBlockSection(const MachineBasicBlock &MBB);
/// Return Label preceding the instruction.
MCSymbol *getLabelBeforeInsn(const MachineInstr *MI);
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 40f4dc2689cdf..4f54152c4527e 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -158,18 +158,10 @@ static cl::bits<PGOMapFeaturesEnum> PgoAnalysisMapFeatures(
const char DWARFGroupName[] = "dwarf";
const char DWARFGroupDescription[] = "DWARF Emission";
-const char DbgTimerName[] = "emit";
-const char DbgTimerDescription[] = "Debug Info Emission";
const char EHTimerName[] = "write_exception";
const char EHTimerDescription[] = "DWARF Exception Writer";
const char CFGuardName[] = "Control Flow Guard";
const char CFGuardDescription[] = "Control Flow Guard";
-const char CodeViewLineTablesGroupName[] = "linetables";
-const char CodeViewLineTablesGroupDescription[] = "CodeView Line Tables";
-const char PPTimerName[] = "emit";
-const char PPTimerDescription[] = "Pseudo Probe Emission";
-const char PPGroupName[] = "pseudo probe";
-const char PPGroupDescription[] = "Pseudo Probe Emission";
STATISTIC(EmittedInsts, "Number of machine instrs printed");
@@ -552,28 +544,19 @@ bool AsmPrinter::doInitialization(Module &M) {
if (MAI->doesSupportDebugInformation()) {
bool EmitCodeView = M.getCodeViewFlag();
- if (EmitCodeView && TM.getTargetTriple().isOSWindows()) {
- Handlers.emplace_back(std::make_unique<CodeViewDebug>(this),
- DbgTimerName, DbgTimerDescription,
- CodeViewLineTablesGroupName,
- CodeViewLineTablesGroupDescription);
- }
+ if (EmitCodeView && TM.getTargetTriple().isOSWindows())
+ DebugHandlers.push_back(std::make_unique<CodeViewDebug>(this));
if (!EmitCodeView || M.getDwarfVersion()) {
assert(MMI && "MMI could not be nullptr here!");
if (MMI->hasDebugInfo()) {
DD = new DwarfDebug(this);
- Handlers.emplace_back(std::unique_ptr<DwarfDebug>(DD), DbgTimerName,
- DbgTimerDescription, DWARFGroupName,
- DWARFGroupDescription);
+ DebugHandlers.push_back(std::unique_ptr<DwarfDebug>(DD));
}
}
}
- if (M.getNamedMetadata(PseudoProbeDescMetadataName)) {
- PP = new PseudoProbeHandler(this);
- Handlers.emplace_back(std::unique_ptr<PseudoProbeHandler>(PP), PPTimerName,
- PPTimerDescription, PPGroupName, PPGroupDescription);
- }
+ if (M.getNamedMetadata(PseudoProbeDescMetadataName))
+ PP = std::make_unique<PseudoProbeHandler>(this);
switch (MAI->getExceptionHandlingType()) {
case ExceptionHandling::None:
@@ -640,6 +623,8 @@ bool AsmPrinter::doInitialization(Module &M) {
CFGuardDescription, DWARFGroupName,
DWARFGroupDescription);
+ for (auto &DH : DebugHandlers)
+ DH->beginModule(&M);
for (const HandlerInfo &HI : Handlers) {
NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
HI.TimerGroupDescription, TimePassesIsEnabled);
@@ -791,12 +776,8 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
// sections and expected to be contiguous (e.g. ObjC metadata).
const Align Alignment = getGVAlignment(GV, DL);
- for (const HandlerInfo &HI : Handlers) {
- NamedRegionTimer T(HI.TimerName, HI.TimerDescription,
- HI.TimerGroupName, HI.TimerGroupDescription,
- TimePassesIsEnabled);
- HI.Handler->setSymbolSize(GVSym, Size);
- }
+ for (auto &DH : DebugHandlers)
+ DH->setSymbolSize(GVSym, Size);
// Handle common symbols
if (GVKind.isCommon()) {
@@ -1067,6 +1048,10 @@ void AsmPrinter::emitFunctionHeader() {
}
// Emit pre-function debug and/or EH information.
+ for (auto &DH : DebugHandlers) {
+ DH->beginFunction(MF);
+ DH->beginBasicBlockSection(MF->front());
+ }
for (const HandlerInfo &HI : Handlers) {
NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
HI.TimerGroupDescription, TimePassesIsEnabled);
@@ -1770,11 +1755,8 @@ void AsmPrinter::emitFunctionBody() {
if (MDNode *MD = MI.getPCSections())
emitPCSectionsLabel(*MF, *MD);
- for (const HandlerInfo &HI : Handlers) {
- NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
- HI.TimerGroupDescription, TimePassesIsEnabled);
- HI.Handler->beginInstruction(&MI);
- }
+ for (auto &DH : DebugHandlers)
+ DH->beginInstruction(&MI);
if (isVerbose())
emitComments(MI, OutStreamer->getCommentOS());
@@ -1868,11 +1850,8 @@ void AsmPrinter::emitFunctionBody() {
if (MCSymbol *S = MI.getPostInstrSymbol())
OutStreamer->emitLabel(S);
- for (const HandlerInfo &HI : Handlers) {
- NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
- HI.TimerGroupDescription, TimePassesIsEnabled);
- HI.Handler->endInstruction();
- }
+ for (auto &DH : DebugHandlers)
+ DH->endInstruction();
}
// We must emit temporary symbol for the end of this basic block, if either
@@ -2003,6 +1982,8 @@ void AsmPrinter::emitFunctionBody() {
// Call endBasicBlockSection on the last block now, if it wasn't already
// called.
if (!MF->back().isEndSection()) {
+ for (auto &DH : DebugHandlers)
+ DH->endBasicBlockSection(MF->back());
for (const HandlerInfo &HI : Handlers) {
NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
HI.TimerGroupDescription, TimePassesIsEnabled);
@@ -2022,6 +2003,8 @@ void AsmPrinter::emitFunctionBody() {
emitJumpTableInfo();
// Emit post-function debug and/or EH information.
+ for (auto &DH : DebugHandlers)
+ DH->endFunction(MF);
for (const HandlerInfo &HI : Handlers) {
NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
HI.TimerGroupDescription, TimePassesIsEnabled);
@@ -2463,6 +2446,8 @@ bool AsmPrinter::doFinalization(Module &M) {
emitGlobalIFunc(M, IFunc);
// Finalize debug and EH information.
+ for (auto &DH : DebugHandlers)
+ DH->endModule();
for (const HandlerInfo &HI : Handlers) {
NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
HI.TimerGroupDescription, TimePassesIsEnabled);
@@ -2473,6 +2458,7 @@ bool AsmPrinter::doFinalization(Module &M) {
// keeping all the user-added handlers alive until the AsmPrinter is
// destroyed.
Handlers.erase(Handlers.begin() + NumUserHandlers, Handlers.end());
+ DebugHandlers.clear();
DD = nullptr;
// If the target wants to know about weak references, print them all.
@@ -4059,17 +4045,23 @@ void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) {
// With BB sections, each basic block must handle CFI information on its own
// if it begins a section (Entry block call is handled separately, next to
// beginFunction).
- if (MBB.isBeginSection() && !MBB.isEntryBlock())
+ if (MBB.isBeginSection() && !MBB.isEntryBlock()) {
+ for (auto &DH : DebugHandlers)
+ DH->beginBasicBlockSection(MBB);
for (const HandlerInfo &HI : Handlers)
HI.Handler->beginBasicBlockSection(MBB);
+ }
}
void AsmPrinter::emitBasicBlockEnd(const MachineBasicBlock &MBB) {
// Check if CFI information needs to be updated for this MBB with basic block
// sections.
- if (MBB.isEndSection())
+ if (MBB.isEndSection()) {
+ for (auto &DH : DebugHandlers)
+ DH->endBasicBlockSection(MBB);
for (const HandlerInfo &HI : Handlers)
HI.Handler->endBasicBlockSection(MBB);
+ }
}
void AsmPrinter::emitVisibility(MCSymbol *Sym, unsigned Visibility,
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
index 55d149e049c94..7a138a0332b6d 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
@@ -517,8 +517,6 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
void beginModule(Module *M) override;
- void setSymbolSize(const MCSymbol *, uint64_t) override {}
-
/// Emit the COFF section that holds the line table information.
void endModule() override;
diff --git a/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp b/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
index 24cd1b15a5736..df350b9d4814d 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
@@ -99,6 +99,8 @@ DbgVariableLocation::extractFromMachineInstruction(
DebugHandlerBase::DebugHandlerBase(AsmPrinter *A) : Asm(A), MMI(Asm->MMI) {}
+DebugHandlerBase::~DebugHandlerBase() = default;
+
void DebugHandlerBase::beginModule(Module *M) {
if (M->debug_compile_units().empty())
Asm = nullptr;
diff --git a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
index 234e62506a563..705a61fb827f3 100644
--- a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
+++ b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
@@ -150,11 +150,6 @@ class LLVM_LIBRARY_VISIBILITY EHStreamer : public AsmPrinterHandler {
EHStreamer(AsmPrinter *A);
~EHStreamer() override;
- // Unused.
- void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
- void beginInstruction(const MachineInstr *MI) override {}
- void endInstruction() override {}
-
/// Return `true' if this is a call to a function marked `nounwind'. Return
/// `false' otherwise.
static bool callToNoUnwindFunction(const MachineInstr *MI);
diff --git a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp
index 59c3fa15885e2..5dda38383a656 100644
--- a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp
@@ -20,8 +20,6 @@
using namespace llvm;
-PseudoProbeHandler::~PseudoProbeHandler() = default;
-
void PseudoProbeHandler::emitPseudoProbe(uint64_t Guid, uint64_t Index,
uint64_t Type, uint64_t Attr,
const DILocation *DebugLoc) {
diff --git a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
index a92a89084cadb..c9aaed4800f25 100644
--- a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
+++ b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
@@ -21,7 +21,7 @@ namespace llvm {
class AsmPrinter;
class DILocation;
-class PseudoProbeHandler : public AsmPrinterHandler {
+class PseudoProbeHandler {
// Target of pseudo probe emission.
AsmPrinter *Asm;
// Name to GUID map, used as caching/memoization for speed.
@@ -29,18 +29,9 @@ class PseudoProbeHandler : public AsmPrinterHandler {
public:
PseudoProbeHandler(AsmPrinter *A) : Asm(A){};
- ~PseudoProbeHandler() override;
void emitPseudoProbe(uint64_t Guid, uint64_t Index, uint64_t Type,
uint64_t Attr, const DILocation *DebugLoc);
-
- // Unused.
- void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
- void endModule() override {}
- void beginFunction(const MachineFunction *MF) override {}
- void endFunction(const MachineFunction *MF) override {}
- void beginInstruction(const MachineInstr *MI) override {}
- void endInstruction() override {}
};
} // namespace llvm
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
index 0e472af52c8fa..f94acc912483d 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
+++ b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
@@ -30,8 +30,6 @@ class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmPrinterHandler {
WinCFGuard(AsmPrinter *A);
~WinCFGuard() override;
- void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
-
/// Emit the Control Flow Guard function ID table.
void endModule() override;
@@ -44,12 +42,6 @@ class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmPrinterHandler {
/// Please note that some AsmPrinter implementations may not call
/// beginFunction at all.
void endFunction(const MachineFunction *MF) override;
-
- /// Process beginning of an instruction.
- void beginInstruction(const MachineInstr *MI) override {}
-
- /// Process end of an instruction.
- void endInstruction() override {}
};
} // namespace llvm
diff --git a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp
index c8849bd50464c..c8a47d6d55d7c 100644
--- a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp
+++ b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp
@@ -61,9 +61,7 @@ bool BPFAsmPrinter::doInitialization(Module &M) {
// Only emit BTF when debuginfo available.
if (MAI->doesSupportDebugInformation() && !M.debug_compile_units().empty()) {
BTF = new BTFDebug(this);
- Handlers.push_back(HandlerInfo(std::unique_ptr<BTFDebug>(BTF), "emit",
- "Debug Info Emission", "BTF",
- "BTF Emission"));
+ DebugHandlers.push_back(std::unique_ptr<BTFDebug>(BTF));
}
return false;
diff --git a/llvm/lib/Target/BPF/BTFDebug.h b/llvm/lib/Target/BPF/BTFDebug.h
index 11a0c59ba6c90..3ef4a85299b65 100644
--- a/llvm/lib/Target/BPF/BTFDebug.h
+++ b/llvm/lib/Target/BPF/BTFDebug.h
@@ -420,8 +420,6 @@ class BTFDebug : public DebugHandlerBase {
return DIToIdMap[Ty];
}
- void setSymbolSize(const MCSymbol *Symbol, uint64_t Size) override {}
-
/// Process beginning of an instruction.
void beginInstruction(const MachineInstr *MI) override;
diff --git a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
index 32319f1e97587..01b0bce80d10e 100644
--- a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
+++ b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
@@ -382,13 +382,10 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase {
public:
TestHandler(AsmPrinterHandlerTest &Test) : Test(Test) {}
virtual ~TestHandler() {}
- virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
virtual void beginModule(Module *M) override { Test.BeginCount++; }
virtual void endModule() override { Test.EndCount++; }
virtual void beginFunction(const MachineFunction *MF) override {}
virtual void endFunction(const MachineFunction *MF) override {}
- virtual void beginInstruction(const MachineInstr *MI) override {}
- virtual void endInstruction() override {}
};
protected:
|
NB: supporting multiple debug handlers slightly regresses performance over my initial approach of zero or one debug handlers (c-t-t). I also don't know if DWARF is actually useful for BPF or whether it is just there because it happened to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aeubanks - any opinions on the compile time improvement here? (worth it? the complexity change isn't huge)
@JDevlieghere mabe you/someone else with debug info interest might have some opinions on how this changes the debug info handler interface - but it's pretty low impact
is the performance benefit from removing the timers or from the virtual call optimization? it would be nice to separate those two changes. I'm not sure if anyone is relying on the timers for profiling |
Removing timers is very beneficial (84ca), removing the virtual function calls has a minor, but still postive effect on the instruction count (46aa).
|
Apologies for asking for extra work, but it would be good to do some git archaeology to see if these timers were intentionally added for someone to use (in which case we should ask around before removing them), or if they were randomly added without much thought |
e8dd284 orginally introduced Dwarf timers in 2009. 6e6d1b2 splitted timers between debug info and exception writing in 2009. f8dba24 moved timers from DwarfDebug to AsmPrinter in 2010. It also introduced timers per instruction (back then called begin/endScope), which is currently the main performance problem. The commit message gives no reason for this. Later, timers were refactored multiple times and kept (NB: 1cd1444 introduced AsmPrinterHandler, generalizing things). I think the timers per instruction are just there, because someone added them. I don't know how much use these timers see, because apart from a few refactoring, they didn't seem to get many changes. Edit: I also don't think anyone of the people who added these timers still work on upstream LLVM. I don't know about downstream projects, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PseudoProbeHandler is no longer an AsmPrinterHandler -- it used nothing of its functionality to hook in at the possible points. This avoids virtual function calls when a pseudo probe printer is present.
LGTM. cleanup after https://reviews.llvm.org/D91878
DebugHandlerBase is no longer an AsmPrinterHandler, but a separate base class. DebugHandlerBase is the only remaining "hook" for begin/end instruction and setSymbolSize (only used by DWARFDebug). begin/end for function and basic block sections are never overriden and therefore are no longer virtual. (Originally I intended there to be only one debug handler, but BPF as the only target supports two at the same time: DWARF and BTF.)
LGTM. This part of this patch yields the largest win.
AsmPrinterHandler no longer has begin/end instruction and setSymbolSize hooks -- these were only used by DebugHandlerBase. This avoid iterating over handlers in every instruction.
LGTM.
Remove NamedRegionTimer from instruction loop. Checking a global variable for every instruction (and doing an out-of-line function call) is too expensive for a profiling functionality.
LGTM. Agreed that the overhead overweighs the potential use.
To address @aeubanks's comment, this could be split to a separate PR.
@@ -208,6 +209,9 @@ class AsmPrinter : public MachineFunctionPass { | |||
std::vector<HandlerInfo> Handlers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal note: HandlerInfo
was made public by https://reviews.llvm.org/D74158 due to Julia's use. @vtjnash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I added an addDebugHandler method so that the Julia use case should continue to work.
I retained timers for now. I will make a separate PR to remove them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
virtual void beginModule(Module *M) override { Test.BeginCount++; } | ||
virtual void endModule() override { Test.EndCount++; } | ||
virtual void beginFunction(const MachineFunction *MF) override {} | ||
virtual void endFunction(const MachineFunction *MF) override {} | ||
virtual void beginInstruction(const MachineInstr *MI) override {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MaskRay. I am concerned that this is removing this particular API that is used by Julia to add a new DebugHandler. I am a mild NAK on doing so merely in the interest in reducing the internal instruction counter metric when it seems that you reported it doesn't actually improve performance. Refactoring is fine, and I don't mind if we need to change names of the API and such, as long as the functionality remains accessible to add beginInstruction hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this purpose, I already added addDebugHandler
where you can add a DebugHandler(Base), which still allows to hook in at every instruction. Could you check whether this replacement suits your needs?
reducing the internal instruction counter metric
That's one way to phrase it -- improving performance of every single compilation done by LLVM would be mine. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instruction count may only have a weak correlation with wall time though, since instructions take vastly different times to retire, while your comment above seemed to show that this PR made everything measured at 0.5% slower (#96785 (comment)) if I am reading that right. I assume that is well within the noise factor, so not a blocker from me.
addDebugHandler
looks good for me, but might need a new test here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instruction count may only have a weak correlation with wall time though
I know and also do time measurements locally. Indirect function calls are frequently less efficient than, e.g., arithmetic instructions.
addDebugHandler looks good for me, but might need a new test here?
Good, thanks for checking! I'll add a test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test for addDebugHandler. If Julia's use case turns out to have significant problems after merging this, we can still adjust DebugHandlerBase afterwards to be more flexible.
@@ -20,6 +20,7 @@ | |||
#include "llvm/ADT/SmallVector.h" | |||
#include "llvm/BinaryFormat/Dwarf.h" | |||
#include "llvm/CodeGen/AsmPrinterHandler.h" | |||
#include "llvm/CodeGen/DebugHandlerBase.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be avoided somehow? It pulls the entire kitchen sink into AsmPrinter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, I don't think so, HandlerInfo is templated and unique_ptr requires the definition for the destructor. After #97046, which removes HandlerInfo, it should be possible with an out-of-line destructor of AsmPrinter. I'll check next week (I won't merge before then anyway) -- thanks for pointing this out.
✅ With the latest revision this PR passed the C/C++ code formatter. |
Currently, an AsmPrinterHandler has several methods that allow to dynamically hook in unwind or debug info emission, e.g. at begin/end of every function or instruction. The class hierarchy and the actually overridden functions are as follows: (SymSz=setSymbolSize, mFE=markFunctionEnd, BBS=BasicBlockSection, FL=Funclet; b=beginX, e=endX) SymSz Mod Fn mFE BBS FL Inst AsmPrinterHandler - - - - - - - ` PseudoProbeHandler - - - - - - - ` WinCFGuard - e e - - - - ` EHStreamer - - - - - - - ` DwarfCFIException - e be - be - - ` ARMException - - be e - - - ` AIXException - - e - - - - ` WinException - e be e - be - ` WasmException - e be - - - - ` DebugHandlerBase - b be - be - be ` BTFDebug - e - - - - b ` CodeViewDebug - be - - - - b ` DWARFDebug yes be - - - - b Doing virtual function calls per instruction is costly and useless when the called function does nothing. This commit performs the following clean-up/improvements: - PseudoProbeHandler is no longer an AsmPrinterHandler -- it used nothing of its functionality to hook in at the possible points. This avoids virtual function calls when a pseudo probe printer is present. - DebugHandlerBase is no longer an AsmPrinterHandler, but a separate base class. DebugHandlerBase is the only remaining "hook" for begin/end instruction and setSymbolSize (only used by DWARFDebug). begin/end for function and basic block sections are never overriden and therefore are no longer virtual. (Originally I intended there to be only one debug handler, but BPF as the only target supports two at the same time: DWARF and BTF.) - AsmPrinterHandler no longer has begin/end instruction and setSymbolSize hooks -- these were only used by DebugHandlerBase. This avoid iterating over handlers in every instruction. AsmPrinterHandler Mod Fn mFE BBS FL ` WinCFGuard e e - - - ` EHStreamer - - - - - ` DwarfCFIException e be - be - ` ARMException - be e - - ` AIXException - e - - - ` WinException e be e - be ` WasmException e be - - - SymSz Mod Fn BBS Inst DebugHandlerBase - b be be be ` BTFDebug - e b ` CodeViewDebug - be b ` DWARFDebug yes be b PseudoProbeHandler (no shared methods) To continue allowing external users (e.g., Julia) to hook in at every instruction, a new method addDebugHandler is exposed. This results in a performance improvement, especially in the -O0 -g0 case with unwind information (e.g., JIT baseline).
Currently, an AsmPrinterHandler has several methods that allow to dynamically hook in unwind or debug info emission, e.g. at begin/end of every function or instruction. The class hierarchy and the actually overridden functions are as follows: (SymSz=setSymbolSize, mFE=markFunctionEnd, BBS=BasicBlockSection, FL=Funclet; b=beginX, e=endX) SymSz Mod Fn mFE BBS FL Inst AsmPrinterHandler - - - - - - - ` PseudoProbeHandler - - - - - - - ` WinCFGuard - e e - - - - ` EHStreamer - - - - - - - ` DwarfCFIException - e be - be - - ` ARMException - - be e - - - ` AIXException - - e - - - - ` WinException - e be e - be - ` WasmException - e be - - - - ` DebugHandlerBase - b be - be - be ` BTFDebug - e - - - - b ` CodeViewDebug - be - - - - b ` DWARFDebug yes be - - - - b Doing virtual function calls per instruction is costly and useless when the called function does nothing. This commit performs the following clean-up/improvements: - PseudoProbeHandler is no longer an AsmPrinterHandler -- it used nothing of its functionality to hook in at the possible points. This avoids virtual function calls when a pseudo probe printer is present. - DebugHandlerBase is no longer an AsmPrinterHandler, but a separate base class. DebugHandlerBase is the only remaining "hook" for begin/end instruction and setSymbolSize (only used by DWARFDebug). begin/end for function and basic block sections are never overriden and therefore are no longer virtual. (Originally I intended there to be only one debug handler, but BPF as the only target supports two at the same time: DWARF and BTF.) - AsmPrinterHandler no longer has begin/end instruction and setSymbolSize hooks -- these were only used by DebugHandlerBase. This avoid iterating over handlers in every instruction. AsmPrinterHandler Mod Fn mFE BBS FL ` WinCFGuard e e - - - ` EHStreamer - - - - - ` DwarfCFIException e be - be - ` ARMException - be e - - ` AIXException - e - - - ` WinException e be e - be ` WasmException e be - - - SymSz Mod Fn BBS Inst DebugHandlerBase - b be be be ` BTFDebug - e b ` CodeViewDebug - be b ` DWARFDebug yes be b PseudoProbeHandler (no shared methods) To continue allowing external users (e.g., Julia) to hook in at every instruction, a new method addDebugHandler is exposed. This results in a performance improvement, especially in the -O0 -g0 case with unwind information (e.g., JIT baseline).
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
This reverts commit 0c0bd04.
… without reverting the performance gains As foreshadowed by the author's comment before merging llvm#96785 (comment), this turned out to be pretty awkward for user handlers, since overriding DebugHandlerBase itself adds a lot of undesirable hidden state and assumptions, which segfault if we overload it wrong. Add an extra hierarchy in the class structure so that we keep the performance gains from splitting up the basic EH handles, without needing to break the API from LLVM v18.
undo breaking changes from "Reduce AsmPrinterHandlers virt. fn calls" without reverting the performance gains As foreshadowed by the author's comment before merging llvm#96785 (comment), this turned out to be pretty awkward for user handlers, since overriding DebugHandlerBase itself adds a lot of undesirable hidden state and assumptions, which segfault if we overload it wrong. Add an extra hierarchy in the class structure so that we keep the performance gains from splitting up the basic EH handles, without needing to break the API from LLVM v18. revert most of the rest, but still expected to be without performance difference
This restores the functionality of AsmPrinterHandlers to what it was prior to #96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of #96785, while restoring the API to the pre-LLVM-19 status quo.
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm/llvm-project#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of #96785, while restoring the API to the pre-LLVM-19 status quo.
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of llvm#96785, while restoring the API to the pre-LLVM-19 status quo.
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of llvm#96785, while restoring the API to the pre-LLVM-19 status quo.
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of llvm#96785, while restoring the API to the pre-LLVM-19 status quo. (cherry picked from commit f6b0555)
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of llvm#96785, while restoring the API to the pre-LLVM-19 status quo. (cherry picked from commit f6b0555)
This restores the functionality of AsmPrinterHandlers to what it was prior to llvm#96785. The attempted hack there of adding a duplicate DebugHandlerBase handling added a lot of hidden state and assumptions, which just segfaulted when we tried to continuing using this API. Instead, this just goes back to the old design, but adds a separate array for the basic EH handles. The duplicate array is identical to the other array of handler, but which doesn't get their begin/endInstruction callbacks called. This still saves the negligible but measurable amount of virtual function calls as was the goal of llvm#96785, while restoring the API to the pre-LLVM-19 status quo. (cherry picked from commit f6b0555)
Currently, an AsmPrinterHandler has several methods that allow to dynamically hook in unwind or debug info emission, e.g. at begin/end of every function or instruction. The class hierarchy and the actually overriden functions are as follows:
Doing virtual function calls per instruction is costly and useless when the called function does nothing.
This commit performs the following clean-up/improvements:
PseudoProbeHandler is no longer an AsmPrinterHandler -- it used nothing of its functionality to hook in at the possible points. This avoids virtual function calls when a pseudo probe printer is present.
DebugHandlerBase is no longer an AsmPrinterHandler, but a separate base class. DebugHandlerBase is the only remaining "hook" for begin/end instruction and setSymbolSize (only used by DWARFDebug). begin/end for function and basic block sections are never overriden and therefore are no longer virtual. (Originally I intended there to be only one debug handler, but BPF as the only target supports two at the same time: DWARF and BTF.)
AsmPrinterHandler no longer has begin/end instruction and setSymbolSize hooks -- these were only used by DebugHandlerBase. This avoid iterating over handlers in every instruction.
Remove NamedRegionTimer from instruction loop. Checking a global variable for every instruction (and doing an out-of-line function call) is too expensive for a profiling functionality.
This results in a performance improvement, especially in the -O0 -g0 case with unwind information (e.g., JIT baseline).
http://llvm-compile-time-tracker.com/compare.php?from=54cb5ca9f48fc542b920662a0eee7c0e6f35bee0&to=9ce05be2903285e1681ced36e5862ad3e155cb17&stat=instructions%3Au