Skip to content

[MC] Use StringTable to reduce dynamic relocations #144202

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
merged 4 commits into from
Jun 25, 2025
Merged

[MC] Use StringTable to reduce dynamic relocations #144202

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
551c831
[MC] Use StringTable to reduce dynamic relocations
rnk Jun 14, 2025
d9e4f0f
fix tblgen tests
rnk Jun 14, 2025
3c05fc7
Merge remote-tracking branch 'origin/main' into strtab-mcsched
rnk Jun 25, 2025
3f6edd2
remove wrong ifdef
rnk Jun 25, 2025
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
14 changes: 13 additions & 1 deletion llvm/include/llvm/MC/MCSchedule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#define LLVM_MC_MCSCHEDULE_H

#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringTable.h"
#include "llvm/MC/MCInstrDesc.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/ErrorHandling.h"
Expand Down Expand Up @@ -124,7 +125,7 @@ struct MCSchedClassDesc {
static const unsigned short VariantNumMicroOps = InvalidNumMicroOps - 1;

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
const char* Name;
uint32_t NameOff;
#endif
uint16_t NumMicroOps : 13;
uint16_t BeginGroup : 1;
Expand Down Expand Up @@ -324,6 +325,9 @@ struct MCSchedModel {
const MCSchedClassDesc *SchedClassTable;
unsigned NumProcResourceKinds;
unsigned NumSchedClasses;
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
const StringTable *SchedClassNames;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the field is declared conditionally here, but then initialized unconditionally both via TableGen and in the test file?

Copy link
Member

@compnerd compnerd Jun 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a concern - hopefully a -Asserts build would just fail and catch this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was an oversight. In the end, there are very few MCSchedModel instances, so having an empty field isn't that important. Fewer ifdefs seems like the better tradeoff.

// Instruction itinerary tables used by InstrItineraryData.
friend class InstrItineraryData;
const InstrItinerary *InstrItineraries;
Expand Down Expand Up @@ -368,6 +372,14 @@ struct MCSchedModel {
return &SchedClassTable[SchedClassIdx];
}

StringRef getSchedClassName(unsigned SchedClassIdx) const {
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
return (*SchedClassNames)[SchedClassTable[SchedClassIdx].NameOff];
#else
return "<unknown>";
#endif
}

/// Returns the latency value for the scheduling class.
LLVM_ABI static int computeInstrLatency(const MCSubtargetInfo &STI,
const MCSchedClassDesc &SCDesc);
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/MC/MCSchedule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const MCSchedModel MCSchedModel::Default = {DefaultIssueWidth,
0,
0,
nullptr,
nullptr,
nullptr};

int MCSchedModel::computeInstrLatency(const MCSubtargetInfo &STI,
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/MCA/InstrBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ static void initializeUsedResources(InstrDesc &ID,
WithColor::warning()
<< "Ignoring invalid write of zero cycles on processor resource "
<< PR.Name << "\n";
WithColor::note() << "found in scheduling class " << SCDesc.Name
WithColor::note() << "found in scheduling class "
<< SM.getSchedClassName(ID.SchedClassID)
<< " (write index #" << I << ")\n";
#endif
continue;
Expand Down
8 changes: 4 additions & 4 deletions llvm/test/TableGen/CompressWriteLatencyEntry.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ def Read_D : SchedRead;
// CHECK-NEXT: }; // MyTargetReadAdvanceTable

// CHECK: static const llvm::MCSchedClassDesc SchedModel_ASchedClasses[] = {
// CHECK-NEXT: {DBGFIELD("InvalidSchedClass") 8191, false, false, false, 0, 0, 0, 0, 0, 0},
// CHECK-NEXT: {DBGFIELD("Inst_A") 1, false, false, false, 0, 0, 1, 1, 0, 0}, // #1
// CHECK-NEXT: {DBGFIELD("Inst_B") 1, false, false, false, 0, 0, 2, 1, 0, 0}, // #2
// CHECK-NEXT: {DBGFIELD("Inst_C") 1, false, false, false, 0, 0, 1, 1, 1, 1}, // #3
// CHECK-NEXT: {DBGFIELD(1) 8191, false, false, false, 0, 0, 0, 0, 0, 0},
// CHECK-NEXT: {DBGFIELD(/*Inst_A*/ {{[0-9]+}}) 1, false, false, false, 0, 0, 1, 1, 0, 0}, // #1
// CHECK-NEXT: {DBGFIELD(/*Inst_B*/ {{[0-9]+}}) 1, false, false, false, 0, 0, 2, 1, 0, 0}, // #2
// CHECK-NEXT: {DBGFIELD(/*Inst_C*/ {{[0-9]+}}) 1, false, false, false, 0, 0, 1, 1, 1, 1}, // #3
// CHECK-NEXT: }; // SchedModel_ASchedClasses

let SchedModel = SchedModel_A in {
Expand Down
22 changes: 11 additions & 11 deletions llvm/test/TableGen/InvalidMCSchedClassDesc.td
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// RUN: llvm-tblgen -gen-subtarget -I %p/../../include %s 2>&1 | FileCheck %s
// Check if it is valid MCSchedClassDesc if didn't have the resources.
// Check if it is valid MCSchedClassDesc if didn't have the resources.

include "llvm/Target/Target.td"

def MyTarget : Target;

let OutOperandList = (outs), InOperandList = (ins) in {
def Inst_A : Instruction;
def Inst_B : Instruction;
def Inst_A : Instruction;
def Inst_B : Instruction;
}

let CompleteModel = 0 in {
Expand All @@ -18,27 +18,27 @@ let CompleteModel = 0 in {

// Inst_B didn't have the resoures, and it is invalid.
// CHECK: SchedModel_ASchedClasses[] = {
// CHECK: {DBGFIELD("Inst_A") 1
// CHECK-NEXT: {DBGFIELD("Inst_B") 8191
// CHECK: {DBGFIELD(/*Inst_A*/ 19) 1
// CHECK-NEXT: {DBGFIELD(/*Inst_B*/ 26) 8191
let SchedModel = SchedModel_A in {
def Write_A : SchedWriteRes<[]>;
def : InstRW<[Write_A], (instrs Inst_A)>;
}

// Inst_A didn't have the resoures, and it is invalid.
// CHECK: SchedModel_BSchedClasses[] = {
// CHECK: {DBGFIELD("Inst_A") 8191
// CHECK-NEXT: {DBGFIELD("Inst_B") 1
// CHECK: {DBGFIELD(/*Inst_A*/ 19) 8191
// CHECK-NEXT: {DBGFIELD(/*Inst_B*/ 26) 1
let SchedModel = SchedModel_B in {
def Write_B: SchedWriteRes<[]>;
def Write_B: SchedWriteRes<[]>;
def : InstRW<[Write_B], (instrs Inst_B)>;
}

// CHECK: SchedModel_CSchedClasses[] = {
// CHECK: {DBGFIELD("Inst_A") 1
// CHECK-NEXT: {DBGFIELD("Inst_B") 1
// CHECK: {DBGFIELD(/*Inst_A*/ 19) 1
// CHECK-NEXT: {DBGFIELD(/*Inst_B*/ 26) 1
let SchedModel = SchedModel_C in {
def Write_C: SchedWriteRes<[]>;
def Write_C: SchedWriteRes<[]>;
def : InstRW<[Write_C], (instrs Inst_A, Inst_B)>;
}

Expand Down
9 changes: 5 additions & 4 deletions llvm/tools/llvm-exegesis/lib/Analysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,9 @@ void Analysis::printInstructionRowCsv(const size_t PointId,
std::tie(SchedClassId, std::ignore) = ResolvedSchedClass::resolveSchedClassId(
State_.getSubtargetInfo(), State_.getInstrInfo(), MCI);
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
const MCSchedClassDesc *const SCDesc =
State_.getSubtargetInfo().getSchedModel().getSchedClassDesc(SchedClassId);
writeEscaped<kEscapeCsv>(OS, SCDesc->Name);
StringRef SCDescName =
State_.getSubtargetInfo().getSchedModel().getSchedClassName(SchedClassId);
writeEscaped<kEscapeCsv>(OS, SCDescName);
#else
OS << SchedClassId;
#endif
Expand Down Expand Up @@ -563,7 +563,8 @@ Error Analysis::run<Analysis::PrintSchedClassInconsistencies>(
OS << "<div class=\"inconsistency\"><p>Sched Class <span "
"class=\"sched-class-name\">";
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
writeEscaped<kEscapeHtml>(OS, RSCAndPoints.RSC.SCDesc->Name);
writeEscaped<kEscapeHtml>(OS, SI.getSchedModel().getSchedClassName(
RSCAndPoints.RSC.SchedClassId));
#else
OS << RSCAndPoints.RSC.SchedClassId;
#endif
Expand Down
21 changes: 17 additions & 4 deletions llvm/utils/TableGen/SubtargetEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "llvm/Support/raw_ostream.h"
#include "llvm/TableGen/Error.h"
#include "llvm/TableGen/Record.h"
#include "llvm/TableGen/StringToOffsetTable.h"
#include "llvm/TableGen/TableGenBackend.h"
#include "llvm/TargetParser/SubtargetFeature.h"
#include <algorithm>
Expand Down Expand Up @@ -1456,6 +1457,10 @@ void SubtargetEmitter::emitSchedClassTables(SchedClassTables &SchedTables,
}
OS << "}; // " << Target << "ReadAdvanceTable\n";

// Pool all SchedClass names in a string table.
StringToOffsetTable StrTab;
unsigned InvalidNameOff = StrTab.GetOrAddStringOffset("InvalidSchedClass");

// Emit a SchedClass table for each processor.
for (const auto &[Idx, Proc] : enumerate(SchedModels.procModels())) {
if (!Proc.hasInstrSchedModel())
Expand All @@ -1473,14 +1478,15 @@ void SubtargetEmitter::emitSchedClassTables(SchedClassTables &SchedTables,
// name and position.
assert(SchedModels.getSchedClass(0).Name == "NoInstrModel" &&
"invalid class not first");
OS << " {DBGFIELD(\"InvalidSchedClass\") "
OS << " {DBGFIELD(" << InvalidNameOff << ") "
<< MCSchedClassDesc::InvalidNumMicroOps
<< ", false, false, false, 0, 0, 0, 0, 0, 0},\n";

for (unsigned SCIdx = 1, SCEnd = SCTab.size(); SCIdx != SCEnd; ++SCIdx) {
MCSchedClassDesc &MCDesc = SCTab[SCIdx];
const CodeGenSchedClass &SchedClass = SchedModels.getSchedClass(SCIdx);
OS << " {DBGFIELD(\"" << SchedClass.Name << "\") ";
unsigned NameOff = StrTab.GetOrAddStringOffset(SchedClass.Name);
OS << " {DBGFIELD(/*" << SchedClass.Name << "*/ " << NameOff << ") ";
if (SchedClass.Name.size() < 18)
OS.indent(18 - SchedClass.Name.size());
OS << MCDesc.NumMicroOps << ", " << (MCDesc.BeginGroup ? "true" : "false")
Expand All @@ -1495,6 +1501,8 @@ void SubtargetEmitter::emitSchedClassTables(SchedClassTables &SchedTables,
}
OS << "}; // " << Proc.ModelName << "SchedClasses\n";
}

StrTab.EmitStringTableDef(OS, Target + "SchedClassNames");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this behind the NDEBUG check as well, or do we just rely on it being DCEd? (Do we need to suppress a warning about an unused static in release builds possibly?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that DCE would occur in the dynamic build. This is an exposed interface, and unless it was marked as infused or static, the linker cannot know it it’s safe to remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want the tablegen output to depend on the assertion mode. For most common builds, this doesn't matter, because tablgen is built using the host configuration, but there are some more advanced build configurations where tablegen is separate, and I wouldn't want to have to figure out how to plumb that configuration option through to trigger a tablegen re-run.

My intention is that we just rely on DCE, since it's trivially dead in the TU. I'm not getting a warning locally in NDEBUG builds, but I'm not sure why, because godbolt code samples show that we should warn in cases like this:
https://godbolt.org/z/jqq8rcz58

That investigation will continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back around to this, it appears that we suppress unused const variable warnings when the variable isn't in the main source file. My test not-minimal test sources looked like this:

// t.cpp
struct StringTable {
    constexpr explicit StringTable(int x) : x(x) {}
    int x;
};
#include "gen.h"
#ifdef NOTDEFINED
#define MAYBEUSEIT(x) x
#else
#define MAYBEUSEIT(x) 0
#endif
int dontUseIt() { return MAYBEUSEIT(unusedThing.x); }

// gen.h
static constexpr StringTable unusedThing{42};

$ clang++ -c -std=c++17 -Wall t.cpp
# no warning

And if you put the unused variable in in place of the include, you get the expected warning:

t.cpp:5:30: warning: unused variable 'unusedThing' [-Wunused-const-variable]
    5 | static constexpr StringTable unusedThing{42};
      |                              ^~~~~~~~~~~
1 warning generated.

So, I think this is all WAI. The suppression makes sense, it's probably intended to avoid warning about static const int myconstant = 42; appearing in a header.

Any other blocking concerns, or is this good to go?

}

void SubtargetEmitter::emitProcessorModels(raw_ostream &OS) {
Expand Down Expand Up @@ -1548,6 +1556,8 @@ void SubtargetEmitter::emitProcessorModels(raw_ostream &OS) {
else
OS << " nullptr, nullptr, 0, 0,"
<< " // No instruction-level machine model.\n";
OS << " DBGVAL_OR_NULLPTR(&" << Target
<< "SchedClassNames), // SchedClassNames\n";
if (PM.hasItineraries())
OS << " " << PM.ItinsDef->getName() << ",\n";
else
Expand All @@ -1569,8 +1579,10 @@ void SubtargetEmitter::emitSchedModel(raw_ostream &OS) {
<< "#endif\n"
<< "#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)\n"
<< "#define DBGFIELD(x) x,\n"
<< "#define DBGVAL_OR_NULLPTR(x) x\n"
<< "#else\n"
<< "#define DBGFIELD(x)\n"
<< "#define DBGVAL_OR_NULLPTR(x) nullptr\n"
<< "#endif\n";

if (SchedModels.hasItineraries()) {
Expand All @@ -1588,10 +1600,11 @@ void SubtargetEmitter::emitSchedModel(raw_ostream &OS) {
}
emitSchedClassTables(SchedTables, OS);

OS << "\n#undef DBGFIELD\n";

// Emit the processor machine model
emitProcessorModels(OS);

OS << "\n#undef DBGFIELD\n";
OS << "\n#undef DBGVAL_OR_NULLPTR\n";
}

static void emitPredicateProlog(const RecordKeeper &Records, raw_ostream &OS) {
Expand Down
Loading