[PATCH] D43680: TableGen: Generalize record types to fix typeIsConvertibleTo et al.
Artem Belevich via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 26 14:58:53 PST 2018
tra added inline comments.
================
Comment at: lib/TableGen/Record.cpp:136
+RecordRecTy *RecordRecTy::get(ArrayRef<Record *> UnsortedClasses) {
+ static FoldingSet<RecordRecTy> ThePool;
+
----------------
TableGen is a library. Having static cache of RecordRecTy will be a problem if the same library is used to process more than one independent tablegen input. Perhaps it should live somewhere in the RecordKeeper?
================
Comment at: lib/TableGen/Record.cpp:210
- for (const auto &SCPair : RTy->getRecord()->getSuperClasses())
- if (Rec->isSubClassOf(SCPair.first))
- return true;
+static RecordRecTy *resolveRecordTypes(RecordRecTy *T1, RecordRecTy *T2) {
+ SmallVector<Record *, 4> CommonSuperClasses;
----------------
This could use a brief description of what this function does (or, perhaps a more descriptive name). It's hard to tell that `resolveRecordTypes` returns a record type built from common ancestors of both types.
================
Comment at: lib/TableGen/Record.cpp:228
+ Stack.push_back(SC);
+ SCs = SCs.drop_back(1 + SC->getSuperClasses().size());
+ }
----------------
This could use an explanation.
If I understand it correctly, we assume that getSuperClasses() returns the list of superclasses as a forest, flattened in preorder. When we drop `1 + SC->getSuperClasses().size()` we remove SC and all its superclasses and we'll descend further when we eventually pop SC off the Stack. It's a neat arrangement, but this is rather fragile as it seems to depend on the details of the implementation. At least I don't see anywhere in the TableGen headers that getSuperClasses guarantees any particular order. At the very least there should be some comments explaining what's going on.
================
Comment at: lib/TableGen/Record.cpp:1410-1415
+ ArrayRef<std::pair<Record *, SMRange>> SCs = Rec->getSuperClasses();
+ while (!SCs.empty()) {
+ Record *SC = SCs.back().first;
+ SuperClasses.push_back(SC);
+ SCs = SCs.drop_back(1 + SC->getSuperClasses().size());
+ }
----------------
Perhaps it's worth extracting this into a `getDirectSuperClasses()` function? It would make `resolveRecordTypes()` easier to understand.
Repository:
rL LLVM
https://reviews.llvm.org/D43680
More information about the llvm-commits
mailing list