[PATCH] D47430: TableGen: Streamline the semantics of NAME

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Perhaps unrelated documentation cleanup can be extracted into a separate patch.



================
Comment at: docs/TableGen/LangRef.rst:247-251
 * a template arg to a ``multiclass``, such as the use of ``Bar`` in::
 
      multiclass Foo<int Bar> {
        int Baz = Bar;
      }
----------------
[doc cleanup candidate] I don't think multiclass supports local variables. 
```
foo.td:2:3: error: expected 'let', 'def', 'defm' or 'foreach' in multiclass body
  int Baz = Bar;
  ^
```




================
Comment at: docs/TableGen/LangRef.rst:387-389
+When ``defm`` occurs in a multiclass and its name does not contain a reference
+to the implicit template argument ``NAME``, such a reference will automatically
+be prepended.
----------------
I'd add an example  similar to `def` here as well.


================
Comment at: include/llvm/TableGen/Record.h:1907
+class HasReferenceResolver final : public Resolver {
+  Init *Needle;
+  bool Found = false;
----------------
However fun puns are, I'd rather use a boring descriptive name. `VarNameToTrack` would make the purpose somewhat more obvious than Needle.


================
Comment at: lib/TableGen/TGParser.cpp:249-250
+  if (CurRec->isClass())
+    Name =
+        VarInit::get(QualifiedNameOfImplicitName(*CurRec), StringRecTy::get());
+  else
----------------
Can this be folded into getNameInit() ?


================
Comment at: lib/TableGen/TGParser.cpp:286-292
+  SmallVector<std::pair<Init *, Init *>, 8> TemplateArgs;
   for (unsigned i = 0, e = SMCTArgs.size(); i != e; ++i) {
     if (i < SubMultiClass.TemplateArgs.size()) {
-      // If a value is specified for this template arg, set it in the
-      // superclass now.
-      if (SetValue(CurRec, SubMultiClass.RefRange.Start, SMCTArgs[i],
-                   None, SubMultiClass.TemplateArgs[i]))
-        return true;
-
-      // If a value is specified for this template arg, set it in the
-      // new defs now.
-      for (const auto &Def :
-             makeArrayRef(CurMC->DefPrototypes).slice(newDefStart)) {
-        if (SetValue(Def.get(), SubMultiClass.RefRange.Start, SMCTArgs[i],
-                     None, SubMultiClass.TemplateArgs[i]))
-          return true;
+      TemplateArgs.emplace_back(SMCTArgs[i], SubMultiClass.TemplateArgs[i]);
+    } else {
+      Init *Default = SMC->Rec.getValue(SMCTArgs[i])->getValue();
+      if (!Default->isComplete()) {
----------------
This could use some comments explaining that this deals with expanding default template arguments.


================
Comment at: lib/TableGen/TGParser.cpp:294
+        return Error(SubMultiClass.RefRange.Start,
+                     "value not specified for template argument #" + Twine(i) +
+                         " (" + SMCTArgs[i]->getAsUnquotedString() +
----------------
Do I understand it correctly that this error will be produced if we can't get complete value for the default argument? If so, then we should probably change the message a bit. Saying "value not specified" for an implicit argument which may intentionally be left unspecified is somewhat confusing. Perhaps something along the lines of "default argument X has unspecified value" would work better?


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:8648
   def : InstAlias<asm # "\t$Vt, [$Rn], #" # Offset,
-                  (!cast<Instruction>(NAME # Count # "v" # layout # "_POST")
+                  (!cast<Instruction>(Name # Count # "v" # layout # "_POST")
                       GPR64sp:$Rn,
----------------
Given that it's an implicit argument of class/multiclass, it should be available here, so there must be another reason for the change.

IIUIC, this is needed because previously NAME would often be resolved to be the name of the top-level def/defm and this is what this code was relying on. Now that NAME is resolved to complete chain of templates from the top level to this multiclass, this no longer works. Is that so?

If this is indeed the case, I'd rename Name->BaseName to make it a tiny bit easier to understand.
I'd also add more details to the patch description as it would help explaining what specifically is wrong with using NAME this way.


Repository:
  rL LLVM

https://reviews.llvm.org/D47430