From fdc2db689530f3b01c78faeb1dd4308e3c128140 Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Wed, 26 Jun 2024 16:30:00 +0200 Subject: [PATCH 1/4] [CodeGen] Reduce AsmPrinterHandlers virt. fn calls 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/include/llvm/CodeGen/AsmPrinter.h | 6 +- llvm/include/llvm/CodeGen/AsmPrinterHandler.h | 10 --- llvm/include/llvm/CodeGen/DebugHandlerBase.h | 26 ++++--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 72 +++++++++---------- llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h | 2 - .../CodeGen/AsmPrinter/DebugHandlerBase.cpp | 2 + llvm/lib/CodeGen/AsmPrinter/EHStreamer.h | 5 -- .../CodeGen/AsmPrinter/PseudoProbePrinter.cpp | 2 - .../CodeGen/AsmPrinter/PseudoProbePrinter.h | 11 +-- llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h | 8 --- llvm/lib/Target/BPF/BPFAsmPrinter.cpp | 4 +- llvm/lib/Target/BPF/BTFDebug.h | 2 - .../unittests/CodeGen/AsmPrinterDwarfTest.cpp | 3 - 13 files changed, 58 insertions(+), 95 deletions(-) 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 Handlers; size_t NumUserHandlers = 0; + /// Debuginfo handler. Protected so that targets can add their own. + SmallVector, 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 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 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(this), - DbgTimerName, DbgTimerDescription, - CodeViewLineTablesGroupName, - CodeViewLineTablesGroupDescription); - } + if (EmitCodeView && TM.getTargetTriple().isOSWindows()) + DebugHandlers.push_back(std::make_unique(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(DD), DbgTimerName, - DbgTimerDescription, DWARFGroupName, - DWARFGroupDescription); + DebugHandlers.push_back(std::unique_ptr(DD)); } } } - if (M.getNamedMetadata(PseudoProbeDescMetadataName)) { - PP = new PseudoProbeHandler(this); - Handlers.emplace_back(std::unique_ptr(PP), PPTimerName, - PPTimerDescription, PPGroupName, PPGroupDescription); - } + if (M.getNamedMetadata(PseudoProbeDescMetadataName)) + PP = std::make_unique(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(BTF), "emit", - "Debug Info Emission", "BTF", - "BTF Emission")); + DebugHandlers.push_back(std::unique_ptr(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: From 841daf6f2746f0f33ec25c54fb913dc365c5f82a Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Fri, 28 Jun 2024 11:12:49 +0000 Subject: [PATCH 2/4] Retain timers for now --- llvm/include/llvm/CodeGen/AsmPrinter.h | 20 ++-- llvm/include/llvm/CodeGen/DebugHandlerBase.h | 2 +- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 104 ++++++++++++------- llvm/lib/Target/BPF/BPFAsmPrinter.cpp | 3 +- 4 files changed, 85 insertions(+), 44 deletions(-) diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h index 317798f25d58c..2e111bf17b821 100644 --- a/llvm/include/llvm/CodeGen/AsmPrinter.h +++ b/llvm/include/llvm/CodeGen/AsmPrinter.h @@ -20,6 +20,7 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/BinaryFormat/Dwarf.h" #include "llvm/CodeGen/AsmPrinterHandler.h" +#include "llvm/CodeGen/DebugHandlerBase.h" #include "llvm/CodeGen/DwarfStringPoolEntry.h" #include "llvm/CodeGen/MachineFunctionPass.h" #include "llvm/CodeGen/StackMaps.h" @@ -40,7 +41,6 @@ class Constant; class ConstantArray; class ConstantPtrAuth; class DataLayout; -class DebugHandlerBase; class DIE; class DIEAbbrev; class DwarfDebug; @@ -146,14 +146,14 @@ class AsmPrinter : public MachineFunctionPass { /// struct HandlerInfo and Handlers permit users or target extended /// AsmPrinter to add their own handlers. - struct HandlerInfo { - std::unique_ptr Handler; + template struct HandlerInfo { + std::unique_ptr Handler; StringRef TimerName; StringRef TimerDescription; StringRef TimerGroupName; StringRef TimerGroupDescription; - HandlerInfo(std::unique_ptr Handler, StringRef TimerName, + HandlerInfo(std::unique_ptr Handler, StringRef TimerName, StringRef TimerDescription, StringRef TimerGroupName, StringRef TimerGroupDescription) : Handler(std::move(Handler)), TimerName(TimerName), @@ -206,11 +206,12 @@ class AsmPrinter : public MachineFunctionPass { /// A vector of all debug/EH info emitters we should use. This vector /// maintains ownership of the emitters. - std::vector Handlers; + SmallVector, 2> Handlers; size_t NumUserHandlers = 0; /// Debuginfo handler. Protected so that targets can add their own. - SmallVector, 1> DebugHandlers; + SmallVector, 1> DebugHandlers; + size_t NumUserDebugHandlers = 0; StackMaps SM; @@ -535,11 +536,16 @@ class AsmPrinter : public MachineFunctionPass { // Overridable Hooks //===------------------------------------------------------------------===// - void addAsmPrinterHandler(HandlerInfo Handler) { + void addAsmPrinterHandler(HandlerInfo Handler) { Handlers.insert(Handlers.begin(), std::move(Handler)); NumUserHandlers++; } + void addDebugHandler(HandlerInfo Handler) { + DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler)); + NumUserDebugHandlers++; + } + // Targets can, or in the case of EmitInstruction, must implement these to // customize output. diff --git a/llvm/include/llvm/CodeGen/DebugHandlerBase.h b/llvm/include/llvm/CodeGen/DebugHandlerBase.h index 0436651a9fd48..36a844e7087fa 100644 --- a/llvm/include/llvm/CodeGen/DebugHandlerBase.h +++ b/llvm/include/llvm/CodeGen/DebugHandlerBase.h @@ -123,7 +123,7 @@ class DebugHandlerBase { public: /// 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 setSymbolSize(const MCSymbol *Sym, uint64_t Size) {} virtual void beginModule(Module *M); virtual void endModule() = 0; diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 4f54152c4527e..e6d77398ee65a 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -158,10 +158,14 @@ static cl::bits 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"; STATISTIC(EmittedInsts, "Number of machine instrs printed"); @@ -544,13 +548,19 @@ bool AsmPrinter::doInitialization(Module &M) { if (MAI->doesSupportDebugInformation()) { bool EmitCodeView = M.getCodeViewFlag(); - if (EmitCodeView && TM.getTargetTriple().isOSWindows()) - DebugHandlers.push_back(std::make_unique(this)); + if (EmitCodeView && TM.getTargetTriple().isOSWindows()) { + DebugHandlers.emplace_back(std::make_unique(this), + DbgTimerName, DbgTimerDescription, + CodeViewLineTablesGroupName, + CodeViewLineTablesGroupDescription); + } if (!EmitCodeView || M.getDwarfVersion()) { assert(MMI && "MMI could not be nullptr here!"); if (MMI->hasDebugInfo()) { DD = new DwarfDebug(this); - DebugHandlers.push_back(std::unique_ptr(DD)); + DebugHandlers.emplace_back(std::unique_ptr(DD), + DbgTimerName, DbgTimerDescription, + DWARFGroupName, DWARFGroupDescription); } } } @@ -623,9 +633,12 @@ bool AsmPrinter::doInitialization(Module &M) { CFGuardDescription, DWARFGroupName, DWARFGroupDescription); - for (auto &DH : DebugHandlers) - DH->beginModule(&M); - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->beginModule(&M); + } + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->beginModule(&M); @@ -776,8 +789,11 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) { // sections and expected to be contiguous (e.g. ObjC metadata). const Align Alignment = getGVAlignment(GV, DL); - for (auto &DH : DebugHandlers) - DH->setSymbolSize(GVSym, Size); + for (auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->setSymbolSize(GVSym, Size); + } // Handle common symbols if (GVKind.isCommon()) { @@ -1048,16 +1064,18 @@ void AsmPrinter::emitFunctionHeader() { } // Emit pre-function debug and/or EH information. - for (auto &DH : DebugHandlers) { - DH->beginFunction(MF); - DH->beginBasicBlockSection(MF->front()); + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->beginFunction(MF); + HI.Handler->beginBasicBlockSection(MF->front()); } - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->beginFunction(MF); } - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->beginBasicBlockSection(MF->front()); @@ -1755,8 +1773,11 @@ void AsmPrinter::emitFunctionBody() { if (MDNode *MD = MI.getPCSections()) emitPCSectionsLabel(*MF, *MD); - for (auto &DH : DebugHandlers) - DH->beginInstruction(&MI); + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->beginInstruction(&MI); + } if (isVerbose()) emitComments(MI, OutStreamer->getCommentOS()); @@ -1850,8 +1871,11 @@ void AsmPrinter::emitFunctionBody() { if (MCSymbol *S = MI.getPostInstrSymbol()) OutStreamer->emitLabel(S); - for (auto &DH : DebugHandlers) - DH->endInstruction(); + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->endInstruction(); + } } // We must emit temporary symbol for the end of this basic block, if either @@ -1982,15 +2006,18 @@ 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) { + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->endBasicBlockSection(MF->back()); + } + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->endBasicBlockSection(MF->back()); } } - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->markFunctionEnd(); @@ -2003,9 +2030,12 @@ void AsmPrinter::emitFunctionBody() { emitJumpTableInfo(); // Emit post-function debug and/or EH information. - for (auto &DH : DebugHandlers) - DH->endFunction(MF); - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->endFunction(MF); + } + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->endFunction(MF); @@ -2446,9 +2476,12 @@ bool AsmPrinter::doFinalization(Module &M) { emitGlobalIFunc(M, IFunc); // Finalize debug and EH information. - for (auto &DH : DebugHandlers) - DH->endModule(); - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : DebugHandlers) { + NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, + HI.TimerGroupDescription, TimePassesIsEnabled); + HI.Handler->endModule(); + } + for (const auto &HI : Handlers) { NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, HI.TimerGroupDescription, TimePassesIsEnabled); HI.Handler->endModule(); @@ -2458,7 +2491,8 @@ 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(); + DebugHandlers.erase(DebugHandlers.begin() + NumUserDebugHandlers, + DebugHandlers.end()); DD = nullptr; // If the target wants to know about weak references, print them all. @@ -3973,7 +4007,7 @@ static void emitBasicBlockLoopComments(const MachineBasicBlock &MBB, void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) { // End the previous funclet and start a new one. if (MBB.isEHFuncletEntry()) { - for (const HandlerInfo &HI : Handlers) { + for (const auto &HI : Handlers) { HI.Handler->endFunclet(); HI.Handler->beginFunclet(MBB); } @@ -4046,9 +4080,9 @@ void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) { // if it begins a section (Entry block call is handled separately, next to // beginFunction). if (MBB.isBeginSection() && !MBB.isEntryBlock()) { - for (auto &DH : DebugHandlers) - DH->beginBasicBlockSection(MBB); - for (const HandlerInfo &HI : Handlers) + for (const auto &HI : DebugHandlers) + HI.Handler->beginBasicBlockSection(MBB); + for (const auto &HI : Handlers) HI.Handler->beginBasicBlockSection(MBB); } } @@ -4057,9 +4091,9 @@ void AsmPrinter::emitBasicBlockEnd(const MachineBasicBlock &MBB) { // Check if CFI information needs to be updated for this MBB with basic block // sections. if (MBB.isEndSection()) { - for (auto &DH : DebugHandlers) - DH->endBasicBlockSection(MBB); - for (const HandlerInfo &HI : Handlers) + for (const auto &HI : DebugHandlers) + HI.Handler->endBasicBlockSection(MBB); + for (const auto &HI : Handlers) HI.Handler->endBasicBlockSection(MBB); } } diff --git a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp index c8a47d6d55d7c..8b929070ffefa 100644 --- a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp +++ b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp @@ -61,7 +61,8 @@ bool BPFAsmPrinter::doInitialization(Module &M) { // Only emit BTF when debuginfo available. if (MAI->doesSupportDebugInformation() && !M.debug_compile_units().empty()) { BTF = new BTFDebug(this); - DebugHandlers.push_back(std::unique_ptr(BTF)); + DebugHandlers.emplace_back(std::unique_ptr(BTF), "emit", + "Debug Info Emission", "BTF", "BTF Emission"); } return false; From eebc570fc697d55dff998cc0f076db7f4d08abbb Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Mon, 1 Jul 2024 09:45:14 +0000 Subject: [PATCH 3/4] Add unit test for addDebugHandler --- .../unittests/CodeGen/AsmPrinterDwarfTest.cpp | 60 ++++++++++++++++++- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp index 01b0bce80d10e..cd0b057f008a8 100644 --- a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp +++ b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp @@ -9,6 +9,8 @@ #include "TestAsmPrinter.h" #include "llvm/BinaryFormat/ELF.h" #include "llvm/CodeGen/AsmPrinter.h" +#include "llvm/CodeGen/AsmPrinterHandler.h" +#include "llvm/CodeGen/DebugHandlerBase.h" #include "llvm/CodeGen/MachineModuleInfo.h" #include "llvm/IR/LegacyPassManager.h" #include "llvm/IR/Module.h" @@ -395,7 +397,7 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase { return false; auto *AP = TestPrinter->getAP(); - AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( + AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( std::unique_ptr(new TestHandler(*this)), "TestTimerName", "TestTimerDesc", "TestGroupName", "TestGroupDesc")); LLVMTargetMachine *LLVMTM = static_cast(&AP->TM); @@ -407,7 +409,7 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase { M->setDataLayout(LLVMTM->createDataLayout()); PM.run(*M); // Now check that we can run it twice. - AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( + AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( std::unique_ptr(new TestHandler(*this)), "TestTimerName", "TestTimerDesc", "TestGroupName", "TestGroupDesc")); PM.run(*M); @@ -426,4 +428,58 @@ TEST_F(AsmPrinterHandlerTest, Basic) { ASSERT_EQ(EndCount, 3); } +class AsmPrinterDebugHandlerTest : public AsmPrinterFixtureBase { + class TestDebugHandler : public DebugHandlerBase { + AsmPrinterDebugHandlerTest &Test; + + public: + TestDebugHandler(AsmPrinterDebugHandlerTest &Test, AsmPrinter *AP) + : DebugHandlerBase(AP), Test(Test) {} + virtual ~TestDebugHandler() {} + virtual void beginModule(Module *M) override { Test.BeginCount++; } + virtual void endModule() override { Test.EndCount++; } + virtual void beginFunctionImpl(const MachineFunction *MF) override {} + virtual void endFunctionImpl(const MachineFunction *MF) override {} + virtual void beginInstruction(const MachineInstr *MI) override {} + virtual void endInstruction() override {} + }; + +protected: + bool init(const std::string &TripleStr, unsigned DwarfVersion, + dwarf::DwarfFormat DwarfFormat) { + if (!AsmPrinterFixtureBase::init(TripleStr, DwarfVersion, DwarfFormat)) + return false; + + auto *AP = TestPrinter->getAP(); + AP->addDebugHandler(AsmPrinter::HandlerInfo( + std::make_unique(*this, AP), "TestTimerName", + "TestTimerDesc", "TestGroupName", "TestGroupDesc")); + LLVMTargetMachine *LLVMTM = static_cast(&AP->TM); + legacy::PassManager PM; + PM.add(new MachineModuleInfoWrapperPass(LLVMTM)); + PM.add(TestPrinter->releaseAP()); // Takes ownership of destroying AP + LLVMContext Context; + std::unique_ptr M(new Module("TestModule", Context)); + M->setDataLayout(LLVMTM->createDataLayout()); + PM.run(*M); + // Now check that we can run it twice. + AP->addDebugHandler(AsmPrinter::HandlerInfo( + std::make_unique(*this, AP), "TestTimerName", + "TestTimerDesc", "TestGroupName", "TestGroupDesc")); + PM.run(*M); + return true; + } + + int BeginCount = 0; + int EndCount = 0; +}; + +TEST_F(AsmPrinterDebugHandlerTest, Basic) { + if (!init("x86_64-pc-linux", /*DwarfVersion=*/4, dwarf::DWARF32)) + GTEST_SKIP(); + + ASSERT_EQ(BeginCount, 3); + ASSERT_EQ(EndCount, 3); +} + } // end namespace From 0b55ca8991edfd97dc7d80de3ac7c50754d25b9c Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Mon, 1 Jul 2024 09:57:09 +0000 Subject: [PATCH 4/4] Satisfy CI clang-format (my local c-f reverts this) --- llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h index c9aaed4800f25..35461e53fbf19 100644 --- a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h +++ b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h @@ -28,7 +28,7 @@ class PseudoProbeHandler { DenseMap NameGuidMap; public: - PseudoProbeHandler(AsmPrinter *A) : Asm(A){}; + PseudoProbeHandler(AsmPrinter *A) : Asm(A) {}; void emitPseudoProbe(uint64_t Guid, uint64_t Index, uint64_t Type, uint64_t Attr, const DILocation *DebugLoc);