[PATCH] D152998: [TableGen] Support named arguments

[Wang Pengcheng via Phabricator via llvm-commits]

wangpc marked 6 inline comments as done.
wangpc added inline comments.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3161
+    // and argument type will be resolved after we know the name.
+    Init *Value = ParseValue(
+        CurRec,
----------------
reames wrote:
> I'm missing something here.  One the *first* named argument, aren't we constructing the value with the wrong type?  Does that matter?
For the first named argument, we are actually parsing the name value (which is a StringInit) here, so the type doesn't matter.
I set nullptr to `ItemType` explictly here for non-first named arguments.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3170
+      if (auto *Name = dyn_cast<StringInit>(Value)) {
+        Init *QualifiedName =
+            QualifyName(*ArgsRec, CurMultiClass, Name, IsDefm ? "::" : ":");
----------------
reames wrote:
> I think this code might be easier to follow if you moved the error checking for name and unset value to the point the arguments are resolved.  This would require that we support both a qualified and unqualified state in the argument name field which is not great.  Unless, can we move resolution to the use of the named argument?  
> 
> (Not at all sure of this proposal, take it as a weak suggestion only.)
When resolving arguments, we can only see `Init`s and `Record`s. We will lose exact error location information if we move the error checking to later parts.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:120
   NewName = BinOpInit::getStrConcat(NewName, Name);
-  if (CurMultiClass && Scoper != "::") {
-    Init *Prefix = BinOpInit::getStrConcat(CurMultiClass->Rec.getNameInit(),
----------------
reames wrote:
> wangpc wrote:
> > @nhaehnle
> > It seems that we can't define nested `class` in `multiclass`, so why do we need this?
> > Do you remember the context in D47431?
> The removal of this code needs to be resolved.  I don't understand the context well enough to address this point.
The removed code assumed that we can define classes inside a multiclass, so the name of outer multiclass is concatenated to the qualified name. But for current TableGen grammar, we can't define classes in multiclass, so it is unnecessary.

The reason why we need to remove it in this patch is:
1. We can define records, invoke class-based subroutines inside a multiclass.
2. When parsing the name of named arguments in `ParseTemplateArgValueList`, we need to construct the qualified name to find if there is an argument with that name.
3. `CurMultiClass` may not be null, so the qualified name woule be like `MultiClass::Class:argname`, which doesn't exist.
4. Then we fail to find the argument which exists actually.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152998/new/

https://reviews.llvm.org/D152998