[PATCH] D47431: TableGen: Allow foreach in multiclass to depend on template args

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.

Few nits. LGTM overall.



================
Comment at: lib/TableGen/TGParser.cpp:392-393
+  bool Error = false;
+  for (unsigned i = 0; i < LI->size(); ++i) {
+    Substs.emplace_back(Loop.IterVar->getNameInit(), LI->getElement(i));
+    Error = resolve(Loop.Entries, Substs, Final, Dest);
----------------
I think `for (auto &E: LI)` should work.


================
Comment at: lib/TableGen/TGParser.cpp:1268-1277
+    std::unique_ptr<Record> ParseRecTmp;
+    Record *ParseRec = CurRec;
+    if (!ParseRec) {
+      ParseRecTmp = make_unique<Record>(".parse", ArrayRef<SMLoc>{}, Records);
+      ParseRec = ParseRecTmp.get();
+    }
+
----------------
This may warrant a comment explaining why we need a temp record here.
AFAICT, it's supposed to provide a scope for the names used in top-level foreach.


================
Comment at: lib/TableGen/TGParser.cpp:1530-1541
+    std::unique_ptr<Record> ParseRecTmp;
+    Record *ParseRec = CurRec;
+    if (!ParseRec) {
+      ParseRecTmp = make_unique<Record>(".parse", ArrayRef<SMLoc>{}, Records);
+      ParseRec = ParseRecTmp.get();
+    }
+
----------------
Same here.


================
Comment at: lib/TableGen/TGParser.h:45-48
+  /// RecordsEntry - Can be either a record or a foreach loop.
+  struct RecordsEntry {
+    std::unique_ptr<Record> Rec;
+    std::unique_ptr<ForeachLoop> Loop;
----------------
Are Rec and Loop expected to be mutually exclusive? 
It would be good to have some asserts enforcing the invariant.




================
Comment at: test/TableGen/NestedForeach.td:20-22
+foreach i = [0, 1] in
+foreach j = !foreach(x, [0, 2], !add(i, x)) in
+def Z#i#_#j;
----------------
Here and below I'd make all nested constructs indented, possibly with braces -- like it's done in the nested `foreach` above.



Repository:
  rL LLVM

https://reviews.llvm.org/D47431