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 14 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
64 changes: 35 additions & 29 deletions clang/utils/TableGen/ClangAttrEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1518,7 +1518,8 @@ createArgument(const Record &Arg, StringRef Attr,

if (!Ptr) {
// Search in reverse order so that the most-derived type is handled first.
for (const auto &[Base, _] : reverse(Search->getSuperClasses())) {
std::vector<const Record *> SCs = Search->getSuperClasses();
for (const Record *Base : reverse(SCs)) {
if ((Ptr = createArgument(Arg, Attr, Base)))
break;
}
Expand Down Expand Up @@ -1848,17 +1849,16 @@ static LateAttrParseKind getLateAttrParseKind(const Record *Attr) {
auto *LAPK = Attr->getValueAsDef(LateParsedStr);

// Typecheck the `LateParsed` field.
SmallVector<const Record *, 1> SuperClasses;
LAPK->getDirectSuperClasses(SuperClasses);
if (SuperClasses.size() != 1)
if (LAPK->getDirectSuperClasses().size() != 1)
PrintFatalError(Attr, "Field `" + Twine(LateParsedStr) +
"`should only have one super class");

if (SuperClasses[0]->getName() != LateAttrParseKindStr)
const Record *SuperClass = LAPK->getDirectSuperClasses()[0].first;
if (SuperClass->getName() != LateAttrParseKindStr)
PrintFatalError(
Attr, "Field `" + Twine(LateParsedStr) + "`should only have type `" +
Twine(LateAttrParseKindStr) + "` but found type `" +
SuperClasses[0]->getName() + "`");
SuperClass->getName() + "`");

// Get Kind and verify the enum name matches the name in `Attr.td`.
unsigned Kind = LAPK->getValueAsInt(KindFieldStr);
Expand Down Expand Up @@ -2457,8 +2457,8 @@ static void emitStringSwitchCases(std::map<StringRef, FSIVecTy> &Map,
}

static bool isTypeArgument(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
Arg->getSuperClasses().back().first->getName() == "TypeArgument";
return !Arg->getDirectSuperClasses().empty() &&
Arg->getDirectSuperClasses().back().first->getName() == "TypeArgument";
}

/// Emits the first-argument-is-type property for attributes.
Expand Down Expand Up @@ -2499,42 +2499,45 @@ static void emitClangAttrArgContextList(const RecordKeeper &Records,
}

static bool isIdentifierArgument(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
return !Arg->getDirectSuperClasses().empty() &&
StringSwitch<bool>(
Arg->getDirectSuperClasses().back().first->getName())
.Case("IdentifierArgument", true)
.Case("EnumArgument", true)
.Case("VariadicEnumArgument", true)
.Default(false);
}

static bool isVariadicIdentifierArgument(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
return !Arg->getDirectSuperClasses().empty() &&
StringSwitch<bool>(
Arg->getDirectSuperClasses().back().first->getName())
.Case("VariadicIdentifierArgument", true)
.Case("VariadicParamOrParamIdxArgument", true)
.Default(false);
}

static bool isVariadicExprArgument(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
return !Arg->getDirectSuperClasses().empty() &&
StringSwitch<bool>(
Arg->getDirectSuperClasses().back().first->getName())
.Case("VariadicExprArgument", true)
.Default(false);
}

static bool isStringLiteralArgument(const Record *Arg) {
if (Arg->getSuperClasses().empty())
if (Arg->getDirectSuperClasses().empty())
return false;
StringRef ArgKind = Arg->getSuperClasses().back().first->getName();
StringRef ArgKind = Arg->getDirectSuperClasses().back().first->getName();
if (ArgKind == "EnumArgument")
return Arg->getValueAsBit("IsString");
return ArgKind == "StringArgument";
}

static bool isVariadicStringLiteralArgument(const Record *Arg) {
if (Arg->getSuperClasses().empty())
if (Arg->getDirectSuperClasses().empty())
return false;
StringRef ArgKind = Arg->getSuperClasses().back().first->getName();
StringRef ArgKind = Arg->getDirectSuperClasses().back().first->getName();
if (ArgKind == "VariadicEnumArgument")
return Arg->getValueAsBit("IsString");
return ArgKind == "VariadicStringArgument";
Expand Down Expand Up @@ -2623,8 +2626,9 @@ static void emitClangAttrStrictIdentifierArgList(const RecordKeeper &Records,
}

static bool keywordThisIsaIdentifierInArgument(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
return !Arg->getDirectSuperClasses().empty() &&
StringSwitch<bool>(
Arg->getDirectSuperClasses().back().first->getName())
.Case("VariadicParamOrParamIdxArgument", true)
.Default(false);
}
Expand Down Expand Up @@ -2710,11 +2714,11 @@ static void emitAttributes(const RecordKeeper &Records, raw_ostream &OS,
if (!R.getValueAsBit("ASTNode"))
continue;

ArrayRef<std::pair<const Record *, SMRange>> Supers = R.getSuperClasses();
std::vector<const Record *> Supers = R.getSuperClasses();
assert(!Supers.empty() && "Forgot to specify a superclass for the attr");
std::string SuperName;
bool Inheritable = false;
for (const auto &[R, _] : reverse(Supers)) {
for (const Record *R : reverse(Supers)) {
if (R->getName() != "TargetSpecificAttr" &&
R->getName() != "DeclOrTypeAttr" && SuperName.empty())
SuperName = R->getName().str();
Expand Down Expand Up @@ -3415,10 +3419,11 @@ namespace {
AttrClass *findSuperClass(const Record *R) const {
// TableGen flattens the superclass list, so we just need to walk it
// in reverse.
auto SuperClasses = R->getSuperClasses();
for (signed i = 0, e = SuperClasses.size(); i != e; ++i) {
auto SuperClass = findClassByRecord(SuperClasses[e - i - 1].first);
if (SuperClass) return SuperClass;
std::vector<const Record *> SuperClasses = R->getSuperClasses();
for (const Record *R : reverse(SuperClasses)) {
AttrClass *SuperClass = findClassByRecord(R);
if (SuperClass)
return SuperClass;
}
return nullptr;
}
Expand Down Expand Up @@ -4618,8 +4623,9 @@ static void GenerateHandleDeclAttribute(const Record &Attr, raw_ostream &OS) {
}

static bool isParamExpr(const Record *Arg) {
return !Arg->getSuperClasses().empty() &&
StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
return !Arg->getDirectSuperClasses().empty() &&
StringSwitch<bool>(
Arg->getDirectSuperClasses().back().first->getName())
.Case("ExprArgument", true)
.Case("VariadicExprArgument", true)
.Default(false);
Expand Down Expand Up @@ -4742,7 +4748,7 @@ void EmitClangAttrParsedAttrImpl(const RecordKeeper &Records, raw_ostream &OS) {
if (Arg->getValueAsBitOrUnset("Fake", UnusedUnset))
continue;
ArgNames.push_back(Arg->getValueAsString("Name").str());
for (const auto &[Class, _] : Arg->getSuperClasses()) {
for (const Record *Class : Arg->getSuperClasses()) {
if (Class->getName().starts_with("Variadic")) {
ArgNames.back().append("...");
break;
Expand Down
4 changes: 2 additions & 2 deletions clang/utils/TableGen/NeonEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2020,8 +2020,8 @@ void NeonEmitter::createIntrinsic(const Record *R,
std::vector<TypeSpec> TypeSpecs = TypeSpec::fromTypeSpecs(Types);

ClassKind CK = ClassNone;
if (R->getSuperClasses().size() >= 2)
CK = ClassMap[R->getSuperClasses()[1].first];
if (!R->getDirectSuperClasses().empty())
CK = ClassMap[R->getDirectSuperClasses()[0].first];

std::vector<std::pair<TypeSpec, TypeSpec>> NewTypeSpecs;
if (!CartesianProductWith.empty()) {
Expand Down
31 changes: 15 additions & 16 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,28 @@ 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.

This example obtains the superclasses of the ``Prototype`` record and then
iterates over the pairs in the returned array.
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

@jayfoad jayfoad Jan 17, 2025

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 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
return a vector of *all* superclasses of a 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
47 changes: 33 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

@mshockwave mshockwave Jan 17, 2025

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

@jayfoad jayfoad Jan 24, 2025

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,30 @@ class Record {
ArrayRef<AssertionInfo> getAssertions() const { return Assertions; }
ArrayRef<DumpInfo> getDumps() const { return Dumps; }

ArrayRef<std::pair<const Record *, SMRange>> getSuperClasses() const {
return SuperClasses;
/// Append all superclasses in post-order to \p Classes.
void getSuperClasses(std::vector<const Record *> &Classes) const {
for (const Record *SC : make_first_range(DirectSuperClasses)) {
SC->getSuperClasses(Classes);
Classes.push_back(SC);
}
}

/// Return all superclasses in post-order.
std::vector<const Record *> getSuperClasses() const {
std::vector<const Record *> Classes;
getSuperClasses(Classes);
return Classes;
}

/// 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

@jayfoad jayfoad Jan 17, 2025

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

@s-barannikov s-barannikov Jan 17, 2025

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

@mshockwave mshockwave Jan 17, 2025

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 +1810,32 @@ class Record {
void checkUnusedTemplateArgs();

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

bool isSubClassOf(StringRef Name) const {
for (const auto &[SC, _] : SuperClasses) {
for (const Record *SC : make_first_range(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.emplace_back(R, Range);
DirectSuperClasses.emplace_back(R, Range);
}

/// If there are any field references that refer to fields that have been
Expand Down
5 changes: 2 additions & 3 deletions llvm/lib/TableGen/DetailedRecordsBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,14 @@ 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();
std::vector<const Record *> Superclasses = Rec.getSuperClasses();
if (Superclasses.empty()) {
OS << " Superclasses: (none)\n";
return;
}

OS << " Superclasses:";
for (const auto &[ClassRec, Loc] : Superclasses) {
for (const Record *ClassRec : Superclasses) {
if (Rec.hasDirectSuperClass(ClassRec))
OS << formatv(" {0}", ClassRec->getNameInitAsString());
else
Expand Down
2 changes: 1 addition & 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,7 @@ 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()) {
for (const Record *SuperClass : Def->getSuperClasses()) {
std::string SuperName = SuperClass->getNameInitAsString();
SuperClasses.push_back(SuperName);
InstanceLists[SuperName].push_back(Name);
Expand Down
Loading
Loading