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

[Artem Belevich via Phabricator via llvm-commits]

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