[PATCH] D43680: TableGen: Generalize record types to fix typeIsConvertibleTo et al.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/TableGen/Record.cpp:136
+RecordRecTy *RecordRecTy::get(ArrayRef<Record *> UnsortedClasses) {
+  static FoldingSet<RecordRecTy> ThePool;
+
----------------
tra wrote:
> 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?
I've moved the pool to the RecordKeeper. Keep in mind that the whole of the TableGen frontend leaks objects in those pools, but at least with this change we can no longer run into the situation where a RecordRecTy is re-used after the underlying Record(s) have been freed and then a new Record was allocated with the same pointer.


================
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;
----------------
tra wrote:
> 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.
Added a comment.


================
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());
+  }
----------------
tra wrote:
> Perhaps it's worth extracting this into a `getDirectSuperClasses()` function? It would make `resolveRecordTypes()` easier to understand.
Good idea, done.


Repository:
  rL LLVM

https://reviews.llvm.org/D43680