Skip to content

[AsmPrinter] Reduce AsmPrinterHandlers virt. fn calls #96785

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.

Sign up for GitHub

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

Merged
aengelke merged 4 commits into from
Jul 1, 2024
Merged

[AsmPrinter] Reduce AsmPrinterHandlers virt. fn calls #96785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fdc2db6
[CodeGen] Reduce AsmPrinterHandlers virt. fn calls
aengelke Jun 26, 2024
841daf6
Retain timers for now
aengelke Jun 28, 2024
eebc570
Add unit test for addDebugHandler
aengelke Jul 1, 2024
0b55ca8
Satisfy CI clang-format (my local c-f reverts this)
aengelke Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion llvm/include/llvm/CodeGen/AsmPrinter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class Constant;
class ConstantArray;
class ConstantPtrAuth;
class DataLayout;
class DebugHandlerBase;
class DIE;
class DIEAbbrev;
class DwarfDebug;
Expand Down Expand Up @@ -208,6 +209,9 @@ class AsmPrinter : public MachineFunctionPass {
std::vector<HandlerInfo> Handlers;
Copy link
Copy Markdown
Member

@MaskRay MaskRay Jun 28, 2024

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

Copy link
Copy Markdown
Contributor Author

@aengelke aengelke Jun 28, 2024

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.

size_t NumUserHandlers = 0;

/// Debuginfo handler. Protected so that targets can add their own.
SmallVector<std::unique_ptr<DebugHandlerBase>, 1> DebugHandlers;

StackMaps SM;

private:
Expand All @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions llvm/include/llvm/CodeGen/AsmPrinterHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
26 changes: 17 additions & 9 deletions llvm/include/llvm/CodeGen/DebugHandlerBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {};
Comment thread
aengelke marked this conversation as resolved.
Outdated

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);
Expand Down
72 changes: 32 additions & 40 deletions llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 0 additions & 5 deletions llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 1 addition & 10 deletions llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,17 @@ 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.
DenseMap<StringRef, uint64_t> NameGuidMap;

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
Expand Down
8 changes: 0 additions & 8 deletions llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down
4 changes: 1 addition & 3 deletions llvm/lib/Target/BPF/BPFAsmPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/Target/BPF/BTFDebug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
3 changes: 0 additions & 3 deletions llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Copy link
Copy Markdown
Member

@vtjnash vtjnash Jun 28, 2024

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.

Copy link
Copy Markdown
Contributor Author

@aengelke aengelke Jun 28, 2024

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. 🤷

Copy link
Copy Markdown
Member

@vtjnash vtjnash Jun 28, 2024

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?

Copy link
Copy Markdown
Contributor Author

@aengelke aengelke Jun 28, 2024

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.

Copy link
Copy Markdown
Contributor Author

@aengelke aengelke Jul 1, 2024

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.

virtual void endInstruction() override {}
};

protected:
Expand Down