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

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/TableGen/TGParser.cpp:249-250
+  if (CurRec->isClass())
+    Name =
+        VarInit::get(QualifiedNameOfImplicitName(*CurRec), StringRecTy::get());
+  else
----------------
nhaehnle wrote:
> 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.
My thinking was that all the information necessary to derive the name is contained in CurRec and so could be done in a CurRec's method (though maybe getNameInit() may not be the right one). Given that there's only one place where we do this, I'm OK to keep it as is.


================
Comment at: lib/TableGen/TGParser.cpp:294
+        return Error(SubMultiClass.RefRange.Start,
+                     "value not specified for template argument #" + Twine(i) +
+                         " (" + SMCTArgs[i]->getAsUnquotedString() +
----------------
nhaehnle wrote:
> 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.
Ah, we're actually dealing with two separate error conditions here. One would be "not enough template arguments" and the other is "we have to provide some default arguments, but can't get the values." OK. I've missed the former.

If there's no easy way to separate those the current error message should be OK to cover both cases.



================
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,
----------------
nhaehnle wrote:
> 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).
OK.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:9418
 multiclass SIMDLdSt1SingleAliases<string asm> {
-  defm : SIMDLdStSingleAliases<asm, "b", "i8",  "One", 1, VectorIndexB>;
-  defm : SIMDLdStSingleAliases<asm, "h", "i16", "One", 2, VectorIndexH>;
-  defm : SIMDLdStSingleAliases<asm, "s", "i32", "One", 4, VectorIndexS>;
-  defm : SIMDLdStSingleAliases<asm, "d", "i64", "One", 8, VectorIndexD>;
+  defm "" : SIMDLdStSingleAliases<asm, "b", "i8",  "One", 1, VectorIndexB>;
+  defm "" : SIMDLdStSingleAliases<asm, "h", "i16", "One", 2, VectorIndexH>;
----------------
`defm ""` pattern is something we should probably highlight in the docs (as examples for def/defm in a multiclass?) because it's somewhat unintuitive. We need to point that defm w/o a name will produce a unique name, while "" is effectively a synonym for NAME (IIUIC, due to rule 2).




Repository:
  rL LLVM

https://reviews.llvm.org/D47430