Skip to content

[TableGen] Only store direct superclasses in Record #123072

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 18 commits into from
Apr 24, 2025
Merged

[TableGen] Only store direct superclasses in Record #123072

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f12511a
Change API of getSuperClasses
jayfoad Jan 15, 2025
2a96ff4
Change API of getDirectSuperClasses
jayfoad Jan 15, 2025
683fe14
Only store direct superclasses in Record
jayfoad Jan 15, 2025
e235152
Simplify API of getDirectSuperClasses
jayfoad Jan 15, 2025
e169673
Update docs
jayfoad Jan 15, 2025
2eb1354
Do not return location info from getSuperClasses
jayfoad Jan 16, 2025
6d74050
Change getSuperClasses to return a std::vector
jayfoad Jan 16, 2025
21492b5
Use const Record *
jayfoad Jan 16, 2025
9a645ee
Add note about post-order
jayfoad Jan 16, 2025
d39a1fc
Use make_first_range
jayfoad Jan 17, 2025
e52de29
Update Clang
jayfoad Jan 17, 2025
9c70b67
Update MLIR
jayfoad Jan 17, 2025
834a337
Tweak Clang changes
jayfoad Jan 17, 2025
2af1d81
Merge remote-tracking branch 'origin/main' into tblgen-superclasses
jayfoad Jan 17, 2025
0889f4c
Update clang/utils/TableGen/ClangAttrEmitter.cpp
jayfoad Jan 24, 2025
e57064b
Fix typo
jayfoad Apr 24, 2025
0160783
Merge remote-tracking branch 'origin/main' into tblgen-superclasses
jayfoad Apr 24, 2025
8bbe3c9
Update MLIR again
jayfoad Apr 24, 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
30 changes: 15 additions & 15 deletions llvm/docs/TableGen/BackGuide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,29 +610,29 @@ functions returns null.
Getting Record Superclasses
===========================

The ``Record`` class provides a function to obtain the superclasses of a
record. It is named ``getSuperClasses`` and returns an ``ArrayRef`` of an
array of ``std::pair`` pairs. The superclasses are in post-order: the order
in which the superclasses were visited while copying their fields into the
record. Each pair consists of a pointer to the ``Record`` instance for a
superclass record and an instance of the ``SMRange`` class. The range
indicates the source file locations of the beginning and end of the class
definition.
The ``Record`` class provides a function to obtain the direct superclasses
of a record. It is named ``getDirectSuperClasses`` and returns an
``ArrayRef`` of an array of ``std::pair`` pairs. Each pair consists of a
pointer to the ``Record`` instance for a superclass record and an instance
of the ``SMRange`` class. The range indicates the source file locations of
the beginning and end of the class definition.
Comment on lines +617 to +618
Copy link
Contributor

@s-barannikov s-barannikov Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not related to this PR, but since you are changing the comment:

The range indicates the source file locations of the beginning and end of the class definition.

Is this true? TGParser::AddSubClass seems to pass the location of a superclass reference in the inheritance list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I noticed this too, and I think you're probably right and the documentation is wrong, but I didn't want to go down the rabbit hole of fact-checking the documentation just now.


This example obtains the superclasses of the ``Prototype`` record and then
iterates over the pairs in the returned array.
This example obtains the direct superclasses of the ``Prototype`` record and
then iterates over the pairs in the returned array.

.. code-block:: text

ArrayRef<std::pair<const Record *, SMRange>>
Superclasses = Prototype->getSuperClasses();
for (const auto &SuperPair : Superclasses) {
Superclasses = Prototype->getDirectSuperClasses();
for (const auto &[Super, Range] : Superclasses) {
...
}

The ``Record`` class also provides a function, ``getDirectSuperClasses``, to
append the *direct* superclasses of a record to a given vector of type
``SmallVectorImpl<Record *>``.
The ``Record`` class also provides a function, ``getSuperClasses``, to
append *all* superclasses of a record to a given vector of type
``SmallVectorImpl<Record *>``. The superclasses are in post-order: the order
in which the superclasses were visited while copying their fields into the
record.

Emitting Text to the Output Stream
==================================
Expand Down
40 changes: 26 additions & 14 deletions llvm/include/llvm/TableGen/Record.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,9 +1630,9 @@ class Record {
SmallVector<AssertionInfo, 0> Assertions;
SmallVector<DumpInfo, 0> Dumps;

// All superclasses in the inheritance forest in post-order (yes, it
// Direct superclasses, which are roots of the inheritance forest (yes, it
// must be a forest; diamond-shaped inheritance is not allowed).
SmallVector<std::pair<const Record *, SMRange>, 0> SuperClasses;
SmallVector<std::pair<const Record *, SMRange>, 0> DirectSuperClasses;
Copy link
Member

Choose a reason for hiding this comment

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

Given the fact that we're only storing a small number of direct super classes now, would it make more sense to use SmallVector<..., N> with N not equal to zero? N = 0 will disable any stack allocation and allocate on heap only, IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried N=1 but it seemed to increase memory usage overall. I think the problem is that Record is used for too many different things, some of which have superclasses and some do not. I would much prefer that we use more specialized types. Most importantly I think "static" objects like class statements and def statements should be represented differently from "runtime" objects like instantiated defs.


// Tracks Record instances. Not owned by Record.
RecordKeeper &TrackedRecords;
Expand Down Expand Up @@ -1666,8 +1666,9 @@ class Record {
Record(const Record &O)
: Name(O.Name), Locs(O.Locs), TemplateArgs(O.TemplateArgs),
Values(O.Values), Assertions(O.Assertions),
SuperClasses(O.SuperClasses), TrackedRecords(O.TrackedRecords),
ID(getNewUID(O.getRecords())), Kind(O.Kind) {}
DirectSuperClasses(O.DirectSuperClasses),
TrackedRecords(O.TrackedRecords), ID(getNewUID(O.getRecords())),
Kind(O.Kind) {}

static unsigned getNewUID(RecordKeeper &RK);

Expand Down Expand Up @@ -1718,15 +1719,23 @@ class Record {
ArrayRef<AssertionInfo> getAssertions() const { return Assertions; }
ArrayRef<DumpInfo> getDumps() const { return Dumps; }

ArrayRef<std::pair<const Record *, SMRange>> getSuperClasses() const {
return SuperClasses;
void getSuperClasses(
SmallVectorImpl<std::pair<const Record *, SMRange>> &Classes) const {
for (const auto &[SC, R] : DirectSuperClasses) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use _ instead of R like you did in isSubClassOf?

Or can we use make_first_range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for make_first_range. Done.

SC->getSuperClasses(Classes);
Classes.emplace_back(SC, R);
}
}

/// Determine whether this record has the specified direct superclass.
bool hasDirectSuperClass(const Record *SuperClass) const;
bool hasDirectSuperClass(const Record *SuperClass) const {
return is_contained(make_first_range(DirectSuperClasses), SuperClass);
}

/// Append the direct superclasses of this record to Classes.
void getDirectSuperClasses(SmallVectorImpl<const Record *> &Classes) const;
/// Return the direct superclasses of this record.
ArrayRef<std::pair<const Record *, SMRange>> getDirectSuperClasses() const {
Copy link
Contributor

@s-barannikov s-barannikov Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(idea for a future patch)
It looks like no clients currently make use of the returned SMRange. It may make sense to provide two methods getDirectSuperClasses returning records and getDirectSuperClassReferenceLocs returning superclass reference locations. (This would require storing records/locations separately or some map_range magic).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like no clients currently make use of the returned SMRange.

CodeGenRegisters uses it, but it looks like it is just trying to clone a Record, and there may be better ways to reimplement that.

Copy link
Contributor Author

@jayfoad jayfoad Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like no clients currently make use of the returned SMRange.

Oh I see, it is copied around but ultimately never used for anything. I can do a follow up to remove it completely.

Or maybe doing that change first, before landing this one, would be less disruptive (in case there are any out-of-tree users of getSuperClasses/getDirectSuperClasses)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure location should be removed completely as it might be used for diagnostics.
On the other hand, I can't immediately think of a case where the location of a super-class reference in an error message would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure location should be removed completely as it might be used for diagnostics.
On the other hand, I can't immediately think of a case where the location of a super-class reference in an error message would be useful.

Though unfortunately we're not doing it at this moment, I think we should make good uses of this location (where the superclass was reference) to show a nice backtrace. Personally I feel like if it doesn't take a lot of efforts we should probably keep the location.

return DirectSuperClasses;
}

bool isTemplateArg(const Init *Name) const {
return llvm::is_contained(TemplateArgs, Name);
Expand Down Expand Up @@ -1794,29 +1803,32 @@ class Record {
void checkUnusedTemplateArgs();

bool isSubClassOf(const Record *R) const {
for (const auto &[SC, _] : SuperClasses)
if (SC == R)
for (const auto &[SC, _] : DirectSuperClasses) {
if (SC == R || SC->isSubClassOf(R))
return true;
}
return false;
}

bool isSubClassOf(StringRef Name) const {
for (const auto &[SC, _] : SuperClasses) {
for (const auto &[SC, _] : DirectSuperClasses) {
if (const auto *SI = dyn_cast<StringInit>(SC->getNameInit())) {
if (SI->getValue() == Name)
return true;
} else if (SC->getNameInitAsString() == Name) {
return true;
}
if (SC->isSubClassOf(Name))
return true;
}
return false;
}

void addSuperClass(const Record *R, SMRange Range) {
void addDirectSuperClass(const Record *R, SMRange Range) {
assert(!CorrespondingDefInit &&
"changing type of record after it has been referenced");
assert(!isSubClassOf(R) && "Already subclassing record!");
SuperClasses.push_back(std::make_pair(R, Range));
DirectSuperClasses.emplace_back(R, Range);
}

/// If there are any field references that refer to fields that have been
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/TableGen/DetailedRecordsBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ void DetailedRecordsEmitter::printTemplateArgs(const Record &Rec,
// are enclosed in parentheses.
void DetailedRecordsEmitter::printSuperclasses(const Record &Rec,
raw_ostream &OS) {
ArrayRef<std::pair<const Record *, SMRange>> Superclasses =
Rec.getSuperClasses();
SmallVector<std::pair<const Record *, SMRange>> Superclasses;
Rec.getSuperClasses(Superclasses);
if (Superclasses.empty()) {
OS << " Superclasses: (none)\n";
return;
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/TableGen/JSONBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ void JSONEmitter::run(raw_ostream &OS) {

json::Array SuperClasses;
// Add this def to the instance list for each of its superclasses.
for (const auto &[SuperClass, Loc] : Def->getSuperClasses()) {
SmallVector<std::pair<const Record *, SMRange>> SCs;
Def->getSuperClasses(SCs);
for (const auto &[SuperClass, Loc] : SCs) {
std::string SuperName = SuperClass->getNameInitAsString();
SuperClasses.push_back(SuperName);
InstanceLists[SuperName].push_back(Name);
Expand Down
43 changes: 6 additions & 37 deletions llvm/lib/TableGen/Record.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ static const RecordRecTy *resolveRecordTypes(const RecordRecTy *T1,
if (T2->isSubClassOf(R)) {
CommonSuperClasses.push_back(R);
} else {
R->getDirectSuperClasses(Stack);
append_range(Stack, make_first_range(R->getDirectSuperClasses()));
}
}

Expand Down Expand Up @@ -2394,11 +2394,8 @@ const DefInit *VarDefInit::instantiate() {

NewRec->resolveReferences(R);

// Add superclasses.
for (const auto &[SC, Loc] : Class->getSuperClasses())
NewRec->addSuperClass(SC, Loc);

NewRec->addSuperClass(
// Add superclass.
NewRec->addDirectSuperClass(
Class, SMRange(Class->getLoc().back(), Class->getLoc().back()));

// Resolve internal references and store in record keeper
Expand Down Expand Up @@ -2874,7 +2871,7 @@ void Record::checkName() {

const RecordRecTy *Record::getType() const {
SmallVector<const Record *, 4> DirectSCs;
getDirectSuperClasses(DirectSCs);
append_range(DirectSCs, make_first_range(getDirectSuperClasses()));
return RecordRecTy::get(TrackedRecords, DirectSCs);
}

Expand Down Expand Up @@ -2906,35 +2903,6 @@ void Record::setName(const Init *NewName) {
// this. See TGParser::ParseDef and TGParser::ParseDefm.
}

// NOTE for the next two functions:
// Superclasses are in post-order, so the final one is a direct
// superclass. All of its transitive superclases immediately precede it,
// so we can step through the direct superclasses in reverse order.

bool Record::hasDirectSuperClass(const Record *Superclass) const {
ArrayRef<std::pair<const Record *, SMRange>> SCs = getSuperClasses();

for (int I = SCs.size() - 1; I >= 0; --I) {
const Record *SC = SCs[I].first;
if (SC == Superclass)
return true;
I -= SC->getSuperClasses().size();
}

return false;
}

void Record::getDirectSuperClasses(
SmallVectorImpl<const Record *> &Classes) const {
ArrayRef<std::pair<const Record *, SMRange>> SCs = getSuperClasses();

while (!SCs.empty()) {
const Record *SC = SCs.back().first;
SCs = SCs.drop_back(1 + SC->getSuperClasses().size());
Classes.push_back(SC);
}
}

void Record::resolveReferences(Resolver &R, const RecordVal *SkipVal) {
const Init *OldName = getNameInit();
const Init *NewName = Name->resolveReferences(R);
Expand Down Expand Up @@ -3008,7 +2976,8 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, const Record &R) {
}

OS << " {";
ArrayRef<std::pair<const Record *, SMRange>> SC = R.getSuperClasses();
SmallVector<std::pair<const Record *, SMRange>> SC;
R.getSuperClasses(SC);
if (!SC.empty()) {
OS << "\t//";
for (const auto &[SC, _] : SC)
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/TableGen/SetTheory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,9 @@ const RecVec *SetTheory::expand(const Record *Set) {
return &I->second;

// This is the first time we see Set. Find a suitable expander.
for (const auto &[SuperClass, Loc] : Set->getSuperClasses()) {
SmallVector<std::pair<const Record *, SMRange>> SCs;
Set->getSuperClasses(SCs);
for (const auto &[SuperClass, Loc] : SCs) {
// Skip unnamed superclasses.
if (!isa<StringInit>(SuperClass->getNameInit()))
continue;
Expand Down
11 changes: 2 additions & 9 deletions llvm/lib/TableGen/TGParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,17 +330,10 @@ bool TGParser::AddSubClass(Record *CurRec, SubClassReference &SubClass) {

// Since everything went well, we can now set the "superclass" list for the
// current record.
for (const auto &[SC, Loc] : SC->getSuperClasses()) {
if (CurRec->isSubClassOf(SC))
return Error(SubClass.RefRange.Start,
"Already subclass of '" + SC->getName() + "'!\n");
CurRec->addSuperClass(SC, Loc);
}

if (CurRec->isSubClassOf(SC))
return Error(SubClass.RefRange.Start,
"Already subclass of '" + SC->getName() + "'!\n");
CurRec->addSuperClass(SC, SubClass.RefRange);
CurRec->addDirectSuperClass(SC, SubClass.RefRange);
return false;
}

Expand Down Expand Up @@ -4003,7 +3996,7 @@ bool TGParser::ParseClass() {
if (CurRec) {
// If the body was previously defined, this is an error.
if (!CurRec->getValues().empty() ||
!CurRec->getSuperClasses().empty() ||
!CurRec->getDirectSuperClasses().empty() ||
!CurRec->getTemplateArgs().empty())
return TokError("Class '" + CurRec->getNameInitAsString() +
"' already defined");
Expand Down
11 changes: 6 additions & 5 deletions llvm/utils/TableGen/CallingConvEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,13 @@ void CallingConvEmitter::emitCallingConv(const Record *CC, raw_ostream &O) {
// Emit all of the actions, in order.
for (unsigned I = 0, E = CCActions->size(); I != E; ++I) {
const Record *Action = CCActions->getElementAsRecord(I);
SmallVector<std::pair<const Record *, SMRange>> SCs;
Action->getSuperClasses(SCs);
SwiftAction =
llvm::any_of(Action->getSuperClasses(),
[](const std::pair<const Record *, SMRange> &Class) {
std::string Name = Class.first->getNameInitAsString();
return StringRef(Name).starts_with("CCIfSwift");
});
llvm::any_of(SCs, [](const std::pair<const Record *, SMRange> &Class) {
std::string Name = Class.first->getNameInitAsString();
return StringRef(Name).starts_with("CCIfSwift");
});

O << "\n";
emitAction(Action, indent(2), O);
Expand Down
4 changes: 2 additions & 2 deletions llvm/utils/TableGen/Common/CodeGenRegisters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,8 +703,8 @@ struct TupleExpander : SetTheory::Expander {
"Register tuple redefines register '" + Name + "'.");

// Copy Proto super-classes.
for (const auto &[Super, Loc] : Proto->getSuperClasses())
NewReg->addSuperClass(Super, Loc);
for (const auto &[Super, Loc] : Proto->getDirectSuperClasses())
NewReg->addDirectSuperClass(Super, Loc);

// Copy Proto fields.
for (unsigned i = 0, e = Proto->getValues().size(); i != e; ++i) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/utils/TableGen/SearchableTableEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ void SearchableTableEmitter::run(raw_ostream &OS) {
const Record *SearchableTable = Records.getClass("SearchableTable");
for (auto &NameRec : Records.getClasses()) {
const Record *Class = NameRec.second.get();
if (Class->getSuperClasses().size() != 1 ||
if (Class->getDirectSuperClasses().size() != 1 ||
!Class->isSubClassOf(SearchableTable))
continue;

Expand Down
Loading