[PATCH] D44478: TableGen: Streamline how defs are instantiated

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

nhaehnle added inline comments.


================
Comment at: lib/TableGen/TGParser.cpp:2453
+    CurRec = make_unique<Record>(Records.getNewAnonymousName(), DefLoc, Records,
+                                 true);
+  else
----------------
tra wrote:
> Nit: `/*Anonymous=*/true`.
> 
Done.


================
Comment at: lib/TableGen/TGParser.cpp:2798
   // Keep track of the new generated record definitions.
-  std::vector<Record*> NewRecDefs;
+  std::vector<std::unique_ptr<Record>> NewRecDefs;
 
----------------
tra wrote:
> Number of items in a multiclass is relatively small. Perhaps we could/should use SmallVector here.
Done.


================
Comment at: lib/TableGen/TGParser.cpp:2853-2856
+        // TODO: This MUST happen before template argument resolution. This
+        //       does not make sense and should be changed, but at the time of
+        //       writing, there are existing .td files which rely on this
+        //       implementation detail. It's a bad idea and should be fixed.
----------------
tra wrote:
> It would be good to have an example or a pointer to something specific so whoever gets to read it later has an ises what needs to be fixed. Perhaps it would make sense to add a disabled test case that  would demonstrate the issue.
Yeah, that's absolutely true.

I added a new test case that shows some of the surprising stuff that happens, in particular: the resolution of record names referencing template args depends on the nesting level at which the template arg is substituted, and trying to add prefixes inside multiclasses ends up with some surprises as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D44478