[llvm] undo breaking changes from "Reduce AsmPrinterHandlers virt. fn calls" without reverting the performance gains (PR #122297)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 9 07:19:40 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Jameson Nash (vtjnash)
<details>
<summary>Changes</summary>
As foreshadowed by the author's comment before merging https://github.com/llvm/llvm-project/pull/96785#discussion_r1660779772, 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. 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.
@<!-- -->aengelke does this look okay to you?
---
Full diff: https://github.com/llvm/llvm-project/pull/122297.diff
8 Files Affected:
- (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+6-7)
- (modified) llvm/include/llvm/CodeGen/AsmPrinterHandler.h (+19-2)
- (modified) llvm/include/llvm/CodeGen/DebugHandlerBase.h (+10-18)
- (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+5-9)
- (modified) llvm/lib/CodeGen/AsmPrinter/EHStreamer.h (+1-1)
- (modified) llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h (+1-1)
- (modified) llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h (+1-1)
- (modified) llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp (+3-50)
``````````diff
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index c9a88d7b1c015c..e3e75f6f463c64 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -33,6 +33,7 @@
namespace llvm {
class AddrLabelMap;
+class AsmBasicPrinterHandler;
class AsmPrinterHandler;
class BasicBlock;
class BlockAddress;
@@ -187,13 +188,13 @@ class AsmPrinter : public MachineFunctionPass {
/// For dso_local functions, the current $local alias for the function.
MCSymbol *CurrentFnBeginLocal = nullptr;
- /// A vector of all debug/EH info emitters we should use. This vector
+ /// A vector of all EH info emitters we should use. This vector
/// maintains ownership of the emitters.
- SmallVector<std::unique_ptr<AsmPrinterHandler>, 2> Handlers;
- size_t NumUserHandlers = 0;
+ SmallVector<std::unique_ptr<AsmBasicPrinterHandler>, 2> Handlers;
- /// Debuginfo handler. Protected so that targets can add their own.
- SmallVector<std::unique_ptr<DebugHandlerBase>, 1> DebugHandlers;
+ // A vector of all Debuginfo emitters we should use. Protected so that
+ // targets can add their own.
+ SmallVector<std::unique_ptr<AsmPrinterHandler>, 1> DebugHandlers;
size_t NumUserDebugHandlers = 0;
StackMaps SM;
@@ -527,8 +528,6 @@ class AsmPrinter : public MachineFunctionPass {
void addAsmPrinterHandler(std::unique_ptr<AsmPrinterHandler> Handler);
- void addDebugHandler(std::unique_ptr<DebugHandlerBase> Handler);
-
// Targets can, or in the case of EmitInstruction, must implement these to
// customize output.
diff --git a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
index ed73e618431ded..daf8793f4ddaf6 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
@@ -30,9 +30,9 @@ typedef MCSymbol *ExceptionSymbolProvider(AsmPrinter *Asm,
/// Collects and handles AsmPrinter objects required to build debug
/// or EH information.
-class AsmPrinterHandler {
+class AsmBasicPrinterHandler {
public:
- virtual ~AsmPrinterHandler();
+ virtual ~AsmBasicPrinterHandler();
virtual void beginModule(Module *M) {}
@@ -70,6 +70,23 @@ class AsmPrinterHandler {
virtual void endFunclet() {}
};
+class AsmPrinterHandler : public AsmBasicPrinterHandler {
+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) {}
+
+ /// Process beginning of an instruction.
+ virtual void beginInstruction(const MachineInstr *MI) = 0;
+
+ /// Process end of an instruction.
+ virtual void endInstruction() = 0;
+
+ virtual void beginCodeAlignment(const MachineBasicBlock &MBB) {}
+};
+
} // End of namespace llvm
#endif
diff --git a/llvm/include/llvm/CodeGen/DebugHandlerBase.h b/llvm/include/llvm/CodeGen/DebugHandlerBase.h
index d39e7e68cb2558..f669bd311ff564 100644
--- a/llvm/include/llvm/CodeGen/DebugHandlerBase.h
+++ b/llvm/include/llvm/CodeGen/DebugHandlerBase.h
@@ -50,14 +50,10 @@ 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 {
+class DebugHandlerBase : public AsmPrinterHandler {
protected:
DebugHandlerBase(AsmPrinter *A);
-public:
- virtual ~DebugHandlerBase();
-
-protected:
/// Target of debug info emission.
AsmPrinter *Asm = nullptr;
@@ -120,24 +116,20 @@ class DebugHandlerBase {
private:
InstructionOrdering InstOrdering;
+ // AsmPrinterHandler overrides.
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 beginModule(Module *M);
- virtual void endModule() = 0;
+ virtual ~DebugHandlerBase() override;
- virtual void beginInstruction(const MachineInstr *MI);
- virtual void endInstruction();
+ void beginModule(Module *M) override;
- void beginFunction(const MachineFunction *MF);
- void endFunction(const MachineFunction *MF);
+ void beginInstruction(const MachineInstr *MI) override;
+ void endInstruction() override;
- void beginBasicBlockSection(const MachineBasicBlock &MBB);
- void endBasicBlockSection(const MachineBasicBlock &MBB);
+ void beginFunction(const MachineFunction *MF) override;
+ void endFunction(const MachineFunction *MF) override;
- virtual void beginCodeAlignment(const MachineBasicBlock &MBB) {}
+ void beginBasicBlockSection(const MachineBasicBlock &MBB) override;
+ void endBasicBlockSection(const MachineBasicBlock &MBB) override;
/// 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 3ba45900e45691..5c952be3bce5ca 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -392,7 +392,7 @@ AsmPrinter::AsmPrinter(TargetMachine &tm, std::unique_ptr<MCStreamer> Streamer)
}
AsmPrinter::~AsmPrinter() {
- assert(!DD && Handlers.size() == NumUserHandlers &&
+ assert(!DD && DebugHandlers.size() == NumUserDebugHandlers &&
"Debug/EH info didn't get finalized");
}
@@ -2591,7 +2591,7 @@ bool AsmPrinter::doFinalization(Module &M) {
// This deletes all the ephemeral handlers that AsmPrinter added, while
// keeping all the user-added handlers alive until the AsmPrinter is
// destroyed.
- Handlers.erase(Handlers.begin() + NumUserHandlers, Handlers.end());
+ Handlers.clear();
DebugHandlers.erase(DebugHandlers.begin() + NumUserDebugHandlers,
DebugHandlers.end());
DD = nullptr;
@@ -4411,19 +4411,15 @@ void AsmPrinter::emitStackMaps() {
void AsmPrinter::addAsmPrinterHandler(
std::unique_ptr<AsmPrinterHandler> Handler) {
- Handlers.insert(Handlers.begin(), std::move(Handler));
- NumUserHandlers++;
-}
-
-void AsmPrinter::addDebugHandler(std::unique_ptr<DebugHandlerBase> Handler) {
DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler));
NumUserDebugHandlers++;
}
-/// Pin vtable to this file.
+/// Pin vtables to this file.
+AsmBasicPrinterHandler::~AsmBasicPrinterHandler() = default;
AsmPrinterHandler::~AsmPrinterHandler() = default;
-void AsmPrinterHandler::markFunctionEnd() {}
+void AsmBasicPrinterHandler::markFunctionEnd() {}
// In the binary's "xray_instr_map" section, an array of these function entries
// describes each instrumentation point. When XRay patches your code, the index
diff --git a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
index 705a61fb827f37..93d13f3e6b2036 100644
--- a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
+++ b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
@@ -27,7 +27,7 @@ class MCSymbol;
template <typename T> class SmallVectorImpl;
/// Emits exception handling directives.
-class LLVM_LIBRARY_VISIBILITY EHStreamer : public AsmPrinterHandler {
+class LLVM_LIBRARY_VISIBILITY EHStreamer : public AsmBasicPrinterHandler {
protected:
/// Target of directive emission.
AsmPrinter *Asm;
diff --git a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
index 35461e53fbf19d..47fcbce2560272 100644
--- a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
+++ b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
@@ -14,7 +14,7 @@
#define LLVM_LIB_CODEGEN_ASMPRINTER_PSEUDOPROBEPRINTER_H
#include "llvm/ADT/DenseMap.h"
-#include "llvm/CodeGen/AsmPrinterHandler.h"
+#include "llvm/CodeGen/AsmPrinter.h"
namespace llvm {
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
index f94acc912483d5..9402c22a1b545b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
+++ b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
@@ -20,7 +20,7 @@
namespace llvm {
-class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmPrinterHandler {
+class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmBasicPrinterHandler {
/// Target of directive emission.
AsmPrinter *Asm;
std::vector<const MCSymbol *> LongjmpTargets;
diff --git a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
index dc738d85547bbc..6c08173f786223 100644
--- a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
+++ b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
@@ -384,10 +384,13 @@ 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:
@@ -424,54 +427,4 @@ 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(std::make_unique<TestDebugHandler>(*this, AP));
- TargetMachine *TM = &AP->TM;
- legacy::PassManager PM;
- PM.add(new MachineModuleInfoWrapperPass(TM));
- PM.add(TestPrinter->releaseAP()); // Takes ownership of destroying AP
- LLVMContext Context;
- std::unique_ptr<Module> M(new Module("TestModule", Context));
- M->setDataLayout(TM->createDataLayout());
- PM.run(*M);
- // Now check that we can run it twice.
- AP->addDebugHandler(std::make_unique<TestDebugHandler>(*this, AP));
- 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
``````````
</details>
https://github.com/llvm/llvm-project/pull/122297
More information about the llvm-commits
mailing list