[PATCH] D152998: [TableGen] Support named arguments

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/TableGen/Record.h:495
   Init *Value;
+  union {
+    unsigned Index;
----------------
Drop the union.  The minimal space savings is not worth the awful and hard to debug problems.

You can use a signal value (i.e. -1 on Index) to track the type of the argument.  


================
Comment at: llvm/include/llvm/TableGen/Record.h:521
   Init *getValue() const { return Value; }
-  ArgumentInit *cloneWithValue(Init *Value) const { return get(Value); }
+  unsigned getIndex() const { return Index; }
+  Init *getName() const { return Name; }
----------------
Add asserts on the kind for both this and the following accessor.  


================
Comment at: llvm/lib/TableGen/Record.cpp:385
 void ArgumentInit::Profile(FoldingSetNodeID &ID) const {
-  ProfileArgumentInit(ID, Value);
+  if (isPositional())
+    ProfilePositionalArgumentInit(ID, Index, Value);
----------------
Once you drop the union, this simplifies.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3161
+    // and argument type will be resolved after we know the name.
+    Init *Value = ParseValue(
+        CurRec,
----------------
I'm missing something here.  One the *first* named argument, aren't we constructing the value with the wrong type?  Does that matter?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3170
+      if (auto *Name = dyn_cast<StringInit>(Value)) {
+        Init *QualifiedName =
+            QualifyName(*ArgsRec, CurMultiClass, Name, IsDefm ? "::" : ":");
----------------
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.)


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3189
+        HasNamedArg = true;
+      } else {
+        Error(ValueLoc,
----------------
Please use early-return here.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:120
   NewName = BinOpInit::getStrConcat(NewName, Name);
-  if (CurMultiClass && Scoper != "::") {
-    Init *Prefix = BinOpInit::getStrConcat(CurMultiClass->Rec.getNameInit(),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152998