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

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

nhaehnle added inline comments.


================
Comment at: lib/TableGen/TGParser.cpp:2917
 
-    TemplateArgs.emplace_back(QualifiedNameOfImplicitName(MC), DefmName);
-
-    // Loop over all the def's in the multiclass, instantiating each one.
-    for (const std::unique_ptr<Record> &DefProto : MC->DefPrototypes) {
-      auto CurRec = make_unique<Record>(*DefProto);
-      CurRec->appendLoc(SubClassLoc);
+    Substs.emplace_back(QualifiedNameOfImplicitName(MC), DefmName);
 
----------------
simon_tatham wrote:
> While you're looking at this code, should this be tracking whether the DefmName you're adding to Substs here was explicitly given by the user, or made up anonymously on line 2861?
> 
> I feel as if the end-product record that comes out of the whole chain of defms and foreaches ought to have its 'anonymous' flag set if //any// defm or def on the way to it was anonymous, because in that case some part of the final name will include something TableGen made up.
I'd agree if we were recreating TableGen from scratch.

However, there are some existing TableGen files (e.g. something in X86 about condition codes) that rely on being able to substitute `NAME` reliably in a multiclass that is instantiated by a defm without a name. I did ponder changing those TableGen files accordingly while I did the earlier change (about streamlining NAME resolution), but ultimately decided against it to reduce the churn in existing .td files.

While I also think the behavior with the made-up "anonymous_NNNN" part in names is a bit weird, it's a comparatively minor wart that doesn't **really** hurt anything as far as I can tell.


================
Comment at: test/TableGen/foreach-multiclass.td:20
+
+// CHECK: def B2-1 {
+// CHECK:   int val = 6;
----------------
simon_tatham wrote:
> Hmm, this is generating `-print-records` output that's not legal tablegen input – the minus sign isn't a valid character in identifier names as far as the lexer's concerned. Is that a problem? I think in this situation I'd have expected a complaint from tablegen ("this concatenation would generate an illegal record name"), rather than silently generating this kind of unexpected output which I could imagine causing knock-on confusion in an unwary backend.
Fair point. I'll at least change the test case to remove this. I don't know if we can or should actually add such a check.


Repository:
  rL LLVM

https://reviews.llvm.org/D47431