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

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

nhaehnle marked 10 inline comments as done.
nhaehnle added inline comments.


================
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);
----------------
tra wrote:
> I think `for (auto &E: LI)` should work.
Right you are, changed it (to *LI actually, but that's a detail).


================
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();
+    }
+
----------------
tra wrote:
> 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.
Right, added a comment here and below.


================
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;
----------------
tra wrote:
> Are Rec and Loop expected to be mutually exclusive? 
> It would be good to have some asserts enforcing the invariant.
> 
> 
Good point, done.


================
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;
----------------
tra wrote:
> Here and below I'd make all nested constructs indented, possibly with braces -- like it's done in the nested `foreach` above.
> 
I'm putting the indent, but keeping it without braces (to make sure that path of the parser is covered as well).


Repository:
  rL LLVM

https://reviews.llvm.org/D47431