From 0fb26fa4ed1702d8e99e31db085b3efdd0a29d10 Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Fri, 28 Jun 2024 11:23:23 +0000 Subject: [PATCH 1/3] [AsmPrinter] Remove timers from instruction-loop Doing per-instruction timing is inaccurate and too costly for a profiling functionality --- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 6676a53eb3b02..35d60db5ace7a 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1776,11 +1776,8 @@ void AsmPrinter::emitFunctionBody() { if (MDNode *MD = MI.getPCSections()) emitPCSectionsLabel(*MF, *MD); - for (const auto &HI : DebugHandlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); + for (const auto &HI : DebugHandlers) HI.Handler->beginInstruction(&MI); - } if (isVerbose()) emitComments(MI, OutStreamer->getCommentOS()); @@ -1874,11 +1871,8 @@ void AsmPrinter::emitFunctionBody() { if (MCSymbol *S = MI.getPostInstrSymbol()) OutStreamer->emitLabel(S); - for (const auto &HI : DebugHandlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); + for (const auto &HI : DebugHandlers) HI.Handler->endInstruction(); - } } // We must emit temporary symbol for the end of this basic block, if either From 32c267d90db6d206b4ae5ad07b175897cf5c2290 Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Thu, 27 Jun 2024 06:40:04 +0000 Subject: [PATCH 2/3] [AsmPrinter] Remove timers Timers are an out-of-line function call and a global variable access, here twice per emitted instruction. At this granularity, not only the time results become skewed, but the timers also add a performance overhead when profiling is disabled. Therefore, remove the timers. --- llvm/include/llvm/CodeGen/AsmPrinter.h | 25 +-- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 154 ++++++------------ llvm/lib/Target/BPF/BPFAsmPrinter.cpp | 3 +- .../unittests/CodeGen/AsmPrinterDwarfTest.cpp | 16 +- 4 files changed, 56 insertions(+), 142 deletions(-) diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h index 2e111bf17b821..d876a142cf338 100644 --- a/llvm/include/llvm/CodeGen/AsmPrinter.h +++ b/llvm/include/llvm/CodeGen/AsmPrinter.h @@ -144,23 +144,6 @@ class AsmPrinter : public MachineFunctionPass { using GOTEquivUsePair = std::pair; MapVector GlobalGOTEquivs; - /// struct HandlerInfo and Handlers permit users or target extended - /// AsmPrinter to add their own handlers. - template struct HandlerInfo { - std::unique_ptr Handler; - StringRef TimerName; - StringRef TimerDescription; - StringRef TimerGroupName; - StringRef TimerGroupDescription; - - HandlerInfo(std::unique_ptr Handler, StringRef TimerName, - StringRef TimerDescription, StringRef TimerGroupName, - StringRef TimerGroupDescription) - : Handler(std::move(Handler)), TimerName(TimerName), - TimerDescription(TimerDescription), TimerGroupName(TimerGroupName), - TimerGroupDescription(TimerGroupDescription) {} - }; - // Flags representing which CFI section is required for a function/module. enum class CFISection : unsigned { None = 0, ///< Do not emit either .eh_frame or .debug_frame @@ -206,11 +189,11 @@ class AsmPrinter : public MachineFunctionPass { /// A vector of all debug/EH info emitters we should use. This vector /// maintains ownership of the emitters. - SmallVector, 2> 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; @@ -536,12 +519,12 @@ class AsmPrinter : public MachineFunctionPass { // Overridable Hooks //===------------------------------------------------------------------===// - void addAsmPrinterHandler(HandlerInfo Handler) { + void addAsmPrinterHandler(std::unique_ptr Handler) { Handlers.insert(Handlers.begin(), std::move(Handler)); NumUserHandlers++; } - void addDebugHandler(HandlerInfo Handler) { + void addDebugHandler(std::unique_ptr Handler) { DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler)); NumUserDebugHandlers++; } diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 35d60db5ace7a..b33d10ae45bfa 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -113,7 +113,6 @@ #include "llvm/Support/Format.h" #include "llvm/Support/MathExtras.h" #include "llvm/Support/Path.h" -#include "llvm/Support/Timer.h" #include "llvm/Support/VCSRevision.h" #include "llvm/Support/raw_ostream.h" #include "llvm/Target/TargetLoweringObjectFile.h" @@ -156,17 +155,6 @@ static cl::bits PgoAnalysisMapFeatures( "Enable extended information within the SHT_LLVM_BB_ADDR_MAP that is " "extracted from PGO related analysis.")); -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"); char AsmPrinter::ID = 0; @@ -550,19 +538,13 @@ bool AsmPrinter::doInitialization(Module &M) { if (MAI->doesSupportDebugInformation()) { bool EmitCodeView = M.getCodeViewFlag(); - if (EmitCodeView && TM.getTargetTriple().isOSWindows()) { - DebugHandlers.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); - DebugHandlers.emplace_back(std::unique_ptr(DD), - DbgTimerName, DbgTimerDescription, - DWARFGroupName, DWARFGroupDescription); + DebugHandlers.push_back(std::unique_ptr(DD)); } } } @@ -625,26 +607,16 @@ bool AsmPrinter::doInitialization(Module &M) { break; } if (ES) - Handlers.emplace_back(std::unique_ptr(ES), EHTimerName, - EHTimerDescription, DWARFGroupName, - DWARFGroupDescription); + Handlers.push_back(std::unique_ptr(ES)); // Emit tables for any value of cfguard flag (i.e. cfguard=1 or cfguard=2). if (mdconst::extract_or_null(M.getModuleFlag("cfguard"))) - Handlers.emplace_back(std::make_unique(this), CFGuardName, - CFGuardDescription, DWARFGroupName, - DWARFGroupDescription); + Handlers.push_back(std::make_unique(this)); - 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); - } + for (auto &Handler : DebugHandlers) + Handler->beginModule(&M); + for (auto &Handler : Handlers) + Handler->beginModule(&M); return false; } @@ -791,11 +763,8 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) { // sections and expected to be contiguous (e.g. ObjC metadata). const Align Alignment = getGVAlignment(GV, DL); - for (auto &HI : DebugHandlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); - HI.Handler->setSymbolSize(GVSym, Size); - } + for (auto &Handler : DebugHandlers) + Handler->setSymbolSize(GVSym, Size); // Handle common symbols if (GVKind.isCommon()) { @@ -1066,22 +1035,14 @@ void AsmPrinter::emitFunctionHeader() { } // Emit pre-function debug and/or EH information. - 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 auto &HI : Handlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); - HI.Handler->beginFunction(MF); - } - for (const auto &HI : Handlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); - HI.Handler->beginBasicBlockSection(MF->front()); + for (auto &Handler : DebugHandlers) { + Handler->beginFunction(MF); + Handler->beginBasicBlockSection(MF->front()); } + for (auto &Handler : Handlers) + Handler->beginFunction(MF); + for (auto &Handler : Handlers) + Handler->beginBasicBlockSection(MF->front()); // Emit the prologue data. if (F.hasPrologueData()) @@ -1776,8 +1737,8 @@ void AsmPrinter::emitFunctionBody() { if (MDNode *MD = MI.getPCSections()) emitPCSectionsLabel(*MF, *MD); - for (const auto &HI : DebugHandlers) - HI.Handler->beginInstruction(&MI); + for (auto &Handler : DebugHandlers) + Handler->beginInstruction(&MI); if (isVerbose()) emitComments(MI, OutStreamer->getCommentOS()); @@ -1871,8 +1832,8 @@ void AsmPrinter::emitFunctionBody() { if (MCSymbol *S = MI.getPostInstrSymbol()) OutStreamer->emitLabel(S); - for (const auto &HI : DebugHandlers) - HI.Handler->endInstruction(); + for (auto &Handler : DebugHandlers) + Handler->endInstruction(); } // We must emit temporary symbol for the end of this basic block, if either @@ -2003,22 +1964,13 @@ void AsmPrinter::emitFunctionBody() { // Call endBasicBlockSection on the last block now, if it wasn't already // called. if (!MF->back().isEndSection()) { - 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 auto &HI : Handlers) { - NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName, - HI.TimerGroupDescription, TimePassesIsEnabled); - HI.Handler->markFunctionEnd(); + for (auto &Handler : DebugHandlers) + Handler->endBasicBlockSection(MF->back()); + for (auto &Handler : Handlers) + Handler->endBasicBlockSection(MF->back()); } + for (auto &Handler : Handlers) + Handler->markFunctionEnd(); MBBSectionRanges[MF->front().getSectionIDNum()] = MBBSectionRange{CurrentFnBegin, CurrentFnEnd}; @@ -2027,16 +1979,10 @@ void AsmPrinter::emitFunctionBody() { emitJumpTableInfo(); // Emit post-function debug and/or EH information. - 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); - } + for (auto &Handler : DebugHandlers) + Handler->endFunction(MF); + for (auto &Handler : Handlers) + Handler->endFunction(MF); // Emit section containing BB address offsets and their metadata, when // BB labels are requested for this function. Skip empty functions. @@ -2473,16 +2419,10 @@ bool AsmPrinter::doFinalization(Module &M) { emitGlobalIFunc(M, IFunc); // Finalize debug and EH information. - 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(); - } + for (auto &Handler : DebugHandlers) + Handler->endModule(); + for (auto &Handler : Handlers) + Handler->endModule(); // This deletes all the ephemeral handlers that AsmPrinter added, while // keeping all the user-added handlers alive until the AsmPrinter is @@ -4004,9 +3944,9 @@ 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 auto &HI : Handlers) { - HI.Handler->endFunclet(); - HI.Handler->beginFunclet(MBB); + for (auto &Handler : Handlers) { + Handler->endFunclet(); + Handler->beginFunclet(MBB); } } @@ -4077,10 +4017,10 @@ 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 (const auto &HI : DebugHandlers) - HI.Handler->beginBasicBlockSection(MBB); - for (const auto &HI : Handlers) - HI.Handler->beginBasicBlockSection(MBB); + for (auto &Handler : DebugHandlers) + Handler->beginBasicBlockSection(MBB); + for (auto &Handler : Handlers) + Handler->beginBasicBlockSection(MBB); } } @@ -4088,10 +4028,10 @@ 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 (const auto &HI : DebugHandlers) - HI.Handler->endBasicBlockSection(MBB); - for (const auto &HI : Handlers) - HI.Handler->endBasicBlockSection(MBB); + for (auto &Handler : DebugHandlers) + Handler->endBasicBlockSection(MBB); + for (auto &Handler : Handlers) + Handler->endBasicBlockSection(MBB); } } diff --git a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp index 2e312e136352d..a9ac4f651ea27 100644 --- a/llvm/lib/Target/BPF/BPFAsmPrinter.cpp +++ b/llvm/lib/Target/BPF/BPFAsmPrinter.cpp @@ -62,8 +62,7 @@ bool BPFAsmPrinter::doInitialization(Module &M) { // Only emit BTF when debuginfo available. if (MAI->doesSupportDebugInformation() && !M.debug_compile_units().empty()) { BTF = new BTFDebug(this); - DebugHandlers.emplace_back(std::unique_ptr(BTF), "emit", - "Debug Info Emission", "BTF", "BTF Emission"); + DebugHandlers.push_back(std::unique_ptr(BTF)); } return false; diff --git a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp index cd0b057f008a8..1f3d7a55ee854 100644 --- a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp +++ b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp @@ -397,9 +397,7 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase { return false; auto *AP = TestPrinter->getAP(); - AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( - std::unique_ptr(new TestHandler(*this)), - "TestTimerName", "TestTimerDesc", "TestGroupName", "TestGroupDesc")); + AP->addAsmPrinterHandler(std::make_unique(*this)); LLVMTargetMachine *LLVMTM = static_cast(&AP->TM); legacy::PassManager PM; PM.add(new MachineModuleInfoWrapperPass(LLVMTM)); @@ -409,9 +407,7 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase { M->setDataLayout(LLVMTM->createDataLayout()); PM.run(*M); // Now check that we can run it twice. - AP->addAsmPrinterHandler(AsmPrinter::HandlerInfo( - std::unique_ptr(new TestHandler(*this)), - "TestTimerName", "TestTimerDesc", "TestGroupName", "TestGroupDesc")); + AP->addAsmPrinterHandler(std::make_unique(*this)); PM.run(*M); return true; } @@ -451,9 +447,7 @@ class AsmPrinterDebugHandlerTest : public AsmPrinterFixtureBase { return false; auto *AP = TestPrinter->getAP(); - AP->addDebugHandler(AsmPrinter::HandlerInfo( - std::make_unique(*this, AP), "TestTimerName", - "TestTimerDesc", "TestGroupName", "TestGroupDesc")); + AP->addDebugHandler(std::make_unique(*this, AP)); LLVMTargetMachine *LLVMTM = static_cast(&AP->TM); legacy::PassManager PM; PM.add(new MachineModuleInfoWrapperPass(LLVMTM)); @@ -463,9 +457,7 @@ class AsmPrinterDebugHandlerTest : public AsmPrinterFixtureBase { 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")); + AP->addDebugHandler(std::make_unique(*this, AP)); PM.run(*M); return true; } From bb1f59ed92c3a8cf02047d103e5435c683b29656 Mon Sep 17 00:00:00 2001 From: Alexis Engelke Date: Mon, 1 Jul 2024 13:07:34 +0000 Subject: [PATCH 3/3] Reduce includes of AsmPrinter.h --- llvm/include/llvm/CodeGen/AsmPrinter.h | 14 ++++---------- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 11 +++++++++++ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h index d876a142cf338..a60dce30c4a6c 100644 --- a/llvm/include/llvm/CodeGen/AsmPrinter.h +++ b/llvm/include/llvm/CodeGen/AsmPrinter.h @@ -19,8 +19,6 @@ #include "llvm/ADT/MapVector.h" #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" @@ -35,12 +33,14 @@ namespace llvm { class AddrLabelMap; +class AsmPrinterHandler; class BasicBlock; class BlockAddress; class Constant; class ConstantArray; class ConstantPtrAuth; class DataLayout; +class DebugHandlerBase; class DIE; class DIEAbbrev; class DwarfDebug; @@ -519,15 +519,9 @@ class AsmPrinter : public MachineFunctionPass { // Overridable Hooks //===------------------------------------------------------------------===// - void addAsmPrinterHandler(std::unique_ptr Handler) { - Handlers.insert(Handlers.begin(), std::move(Handler)); - NumUserHandlers++; - } + void addAsmPrinterHandler(std::unique_ptr Handler); - void addDebugHandler(std::unique_ptr Handler) { - DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler)); - NumUserDebugHandlers++; - } + void addDebugHandler(std::unique_ptr Handler); // Targets can, or in the case of EmitInstruction, must implement these to // customize output. diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index b33d10ae45bfa..0033f0a3699c9 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -4159,6 +4159,17 @@ void AsmPrinter::emitStackMaps() { SM.serializeToStackMapSection(); } +void AsmPrinter::addAsmPrinterHandler( + std::unique_ptr Handler) { + Handlers.insert(Handlers.begin(), std::move(Handler)); + NumUserHandlers++; +} + +void AsmPrinter::addDebugHandler(std::unique_ptr Handler) { + DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler)); + NumUserDebugHandlers++; +} + /// Pin vtable to this file. AsmPrinterHandler::~AsmPrinterHandler() = default;