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

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

nhaehnle added a comment.

Thank you for the review!



================
Comment at: docs/TableGen/LangRef.rst:151
+to the name of the instantiating ``def`` or ``defm``. The result is undefined
+if the class is instantiated by an anonymous record.
 
----------------
simon_tatham wrote:
> I haven't fully got my head round all of this, but this new description of `NAME` doesn't quite seem to match the text in index.rst:
> 
> > Each `def` record has a special entry called "NAME". This is the name of the record ("`ADD32rr`" above). In the general case `def` names can be formed from various kinds of string processing expressions and `NAME` resolves to the final value obtained after resolving all of those expressions. The user may refer to `NAME` anywhere she desires to use the ultimate name of the `def`.
> 
> The general intention seems to be the same (if you refer to `NAME` in some expression in a class declaration, then each def that inherits that expression gets it evaluated in terms of that def's own name). But the old text says that `NAME` is also a property of the final def, whereas the new text seems to suggest that it's a more transient entity that never _really_ exists in the def but is just something classes can refer to while everything's being set up.
> 
> In my JSON backend (D46054) I manually filled in the `NAME` field in the JSON dict for every output def, because that text in `index.rst` said it should be there. But if that's not a reflection of reality any more, perhaps I should rename that field to `!name` to mark it as one of the extra pieces of JSON-level metadata?
Right. I don't have a very strong opinion on whether NAME should really occur in the final `def`s, but they currently don't occur consistently and nobody actually uses it, so I lean slightly against having it (which is reflected in the patch). Note that you can always get the name of a record in TableGen by using `!cast<string>(...)`, which is a nice mirror construct of the more common `!cast<ClassName>(some_string)`.

I'm updating the docs accordingly. I think your suggestion for the JSON dict makes sense as well.


================
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;
      }
----------------
tra wrote:
> [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;
>   ^
> ```
> 
> 
Done as a separate patch.


================
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.
----------------
tra wrote:
> I'd add an example  similar to `def` here as well.
Done.


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


================
Comment at: lib/TableGen/TGParser.cpp:249-250
+  if (CurRec->isClass())
+    Name =
+        VarInit::get(QualifiedNameOfImplicitName(*CurRec), StringRecTy::get());
+  else
----------------
tra wrote:
> Can this be folded into getNameInit() ?
Maybe I misunderstand you, but `CurRec->getNameInit()` returns the name of the class here, while what we want is "NAME" but with a scoping prefix because we treat it like a template arg. So if the class name is "Foo", the string we're looking for is "Foo:NAME".

I personally find the whole scoping thing is a bit awkward, but it works fine from what I can tell.


================
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()) {
----------------
tra wrote:
> This could use some comments explaining that this deals with expanding default template arguments.
Done?


================
Comment at: lib/TableGen/TGParser.cpp:294
+        return Error(SubMultiClass.RefRange.Start,
+                     "value not specified for template argument #" + Twine(i) +
+                         " (" + SMCTArgs[i]->getAsUnquotedString() +
----------------
tra wrote:
> 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?
I'm not sure how I feel about this. The common case in which you'd run into this error is if you forgot a template argument, and for that case, the error message as it currently stands seems more likely to put you on the right track.

The deeper issue here is that we allow ? as an explicit template argument (and I've actually used that, although I could have worked around it), but we don't distinguish between a default template argument that was set to ? (which won't work) and a template argument that simply has no default.

It makes sense to clean that up at some point, but the same issue also exists for class template arguments, and it doesn't really fit into this change.


================
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,
----------------
tra wrote:
> 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.
You understand correctly. I'm making the change for the AArch64 .td file.

I'm sticking with Name in X86InstrAVX512.td, because that file already has precedent for using "Name" for the same purpose (in multiclass avx512_int_broadcastbw_reg).


Repository:
  rL LLVM

https://reviews.llvm.org/D47430