[TableGen] Improve algorithm for inheriting class template arguments and fields

[Paul-C-Anagnostopoulos]

Summary

This revision improves the algorithm that processes a class's template arguments when a record inherits from the class.

Currently the algorithm copies all the template argument defaults into the new record's field list, replaces the values that were specified in the class invocation, copies the values into the resolver map, and then deletes the template arguments from the field list. This new algorithm copies the template arguments directly into the resolver map and then replaces the default values with the specified ones.

This revision relies on a previous revision that made it so field definitions can be tagged as template arguments. It paves the way for a future revision that will handle assert statements in class definitions.

[Paul-C-Anagnostopoulos]

A lot of the changes are in a function named 'AddSubClass', which is a misnomer. It's adding a parent class, not a subclass. Does anyone mind if I rename the function as part of this revision, or should I do it in a separate revision?

[craig.topper]

llvm/include/llvm/TableGen/Record.h

1999

This strongly assumes that Key is in the map with a non-null value. if it isn't the map, it will get inserted with a null V.

Maybe better to make this const, and use Map.find(Key) and assert that it doesn't return Map.end().

llvm/lib/TableGen/TGParser.cpp

2612

Please run clang-format

2628

llvm_unreachable

llvm/test/TableGen/self-reference-typeerror.td

12

nvalid?

[dblaikie]

llvm/lib/TableGen/TGParser.cpp

2628

Or possibly drop the prior "if" and change it to an assert there.

ie change this:

...
else if (x) {
  ...
} else
  llvm_unreachable(...);

to this:

else {
  assert(x);
  ...
}

[Paul-C-Anagnostopoulos]

llvm/include/llvm/TableGen/Record.h

1999

Good point. In the one use so far, it is in the map, but we don't want to assume that.

llvm/lib/TableGen/TGParser.cpp

2612

I've never run it. Doesn't it reformat the entire file?

llvm/test/TableGen/self-reference-typeerror.td

12

So it doesn't matter whether the first word of the message is capitalized.

[craig.topper]

llvm/lib/TableGen/TGParser.cpp

2612

You can run it on only the things changed by your commit by doing this

git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1

I think clang-format-diff is in clang/tools/clang-format in your source tree. There's a -binary option to give it the path to the clang-format binary if its not in your path.

https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

[Paul-C-Anagnostopoulos]

I incorporated the various suggestions.

I had no luck running clang-format-diff, but I will work on it for future revisions.

[Paul-C-Anagnostopoulos]

A little bump to get approval.

[lattner]

This looks fine to me in a quick review, but you're the best technical expert here Paul.

llvm/include/llvm/TableGen/Record.h

2003

plz remove commented out code.

llvm/lib/TableGen/TGParser.cpp

2628

Yeah I'd recommend dblaikie's pattern so you can use assert.

[foad]

llvm/lib/TableGen/TGParser.cpp

2612

If clang/tools/clang-format/git-clang-format is on your path then you can run git clang-format HEAD^ to only reformat lines that are touched by your topmost commit, then git commit -a --amend --no-edit to incorporate the changes that clang-format made if you like.

llvm/include/llvm/TableGen/Record.h

class MapResolver final : public Resolver {
 
   DenseMap<Init *, MappedValue> Map;
 
 public:
   explicit MapResolver(Record *CurRec = nullptr) : Resolver(CurRec) {}
 
   void set(Init *Key, Init *Value) { Map[Key] = {Value, false}; }
 
+  bool isComplete(Init *VarName) const {
+    auto It = Map.find(VarName);
+    assert(It != Map.end() && "key must be present in map");
+    return It->second.V->isComplete();
+//// return Map[Key].V->isComplete(); }
+  }
+
   Init *resolve(Init *VarName) override;
 };
 
 /// Resolve all variables from a record except for unset variables.
 class RecordResolver final : public Resolver {
   DenseMap<Init *, Init *> Cache;
   SmallVector<Init *, 4> Stack;
 

llvm/lib/TableGen/Record.cpp

for (RecordVal &Value : Values) {
     if (Init *V = Value.getValue()) {
       Init *VR = V->resolveReferences(R);
       if (Value.setValue(VR)) {
         std::string Type;
         if (TypedInit *VRT = dyn_cast<TypedInit>(VR))
           Type =
               (Twine("of type '") + VRT->getType()->getAsString() + "' ").str();
         PrintFatalError(getLoc(), Twine("Invalid value ") + Type +
-                                      "is found when setting '" +
+                                      "found when setting field '" +
                                       Value.getNameInitAsString() +
                                       "' of type '" +
                                       Value.getType()->getAsString() +
                                       "' after resolving references: " +
                                       VR->getAsUnquotedString() + "\n");
       }
     }
   }

llvm/lib/TableGen/TGParser.cpp

bool TGParser::SetValue(Record *CurRec, SMLoc Loc, Init *ValName,
   }
   return false;
 }
 
 /// AddSubClass - Add SubClass as a subclass to CurRec, resolving its template
 /// args as SubClass's template arguments.
 bool TGParser::AddSubClass(Record *CurRec, SubClassReference &SubClass) {
   Record *SC = SubClass.Rec;
-  // Add all of the values in the subclass into the current class.
-  for (const RecordVal &Val : SC->getValues())
-    if (AddValue(CurRec, SubClass.RefRange.Start, Val))
-      return true;
-
-  ArrayRef<Init *> TArgs = SC->getTemplateArgs();
-
-  // Ensure that an appropriate number of template arguments are specified.
-  if (TArgs.size() < SubClass.TemplateArgs.size())
-    return Error(SubClass.RefRange.Start,
-                 "More template args specified than expected");
-
-  // Loop over all of the template arguments, setting them to the specified
-  // value or leaving them as the default if necessary.
   MapResolver R(CurRec);
 
-  for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
-    if (i < SubClass.TemplateArgs.size()) {
-      // If a value is specified for this template arg, set it now.
-      if (SetValue(CurRec, SubClass.RefRange.Start, TArgs[i],
-                   None, SubClass.TemplateArgs[i]))
+  // Loop over all the subclass record's fields. Add template arguments
+  // to the resolver map. Add regular fields to the new record.
+  for (const RecordVal &Field : SC->getValues()) {
+    if (Field.isTemplateArg()) {
+      R.set(Field.getNameInit(), Field.getValue());
+    } else {
+      if (AddValue(CurRec, SubClass.RefRange.Start, Field))
         return true;
-    } else if (!CurRec->getValue(TArgs[i])->getValue()->isComplete()) {
-      return Error(SubClass.RefRange.Start,
-                   "Value not specified for template argument #" +
-                   Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
-                   ") of subclass '" + SC->getNameInitAsString() + "'!");
-    }
+    }
 
-    R.set(TArgs[i], CurRec->getValue(TArgs[i])->getValue());
+  ArrayRef<Init *> TArgs = SC->getTemplateArgs();
+  assert(SubClass.TemplateArgs.size() <= TArgs.size() &&
+         "Too many template arguments allowed");
 
-    CurRec->removeValue(TArgs[i]);
+  // Loop over the template argument names. If a value was specified,
+  // reset the map value. If not and there was no default, complain.
+  for (unsigned I = 0, E = TArgs.size(); I != E; ++I) {
+    if (I < SubClass.TemplateArgs.size())
+      R.set(TArgs[I], SubClass.TemplateArgs[I]);
+    else if (!R.isComplete(TArgs[I]))
+      return Error(SubClass.RefRange.Start,
+                   "Value not specified for template argument #" +
+                       Twine(I) + " (" + TArgs[I]->getAsUnquotedString() +
+                       ") of parent class '" + SC->getNameInitAsString() + "'");
   }
 
   Init *Name;
   if (CurRec->isClass())
     Name =
         VarInit::get(QualifiedNameOfImplicitName(*CurRec), StringRecTy::get());
   else
     Name = CurRec->getNameInit();
while (consume(tgtok::comma)) {
     if (!Result.back()) {
       Result.clear();
       return;
     }
   }
 }
 
 /// ParseDeclaration - Read a declaration, returning the name of field ID, or an
-/// empty string on error.  This can happen in a number of different context's,
-/// including within a def or in the template args for a def (which which case
+/// empty string on error.  This can happen in a number of different contexts,
+/// including within a def or in the template args for a class (in which case
 /// CurRec will be non-null) and within the template args for a multiclass (in
 /// which case CurRec will be null, but CurMultiClass will be set).  This can
 /// also happen within a def that is within a multiclass, which will set both
 /// CurRec and CurMultiClass.
 ///
 ///  Declaration ::= FIELD? Type ID ('=' Value)?
 ///
 Init *TGParser::ParseDeclaration(Record *CurRec,
if (Str == "NAME") {
     TokError("'" + Str + "' is a reserved variable name");
     return nullptr;
   }
 
   SMLoc IdLoc = Lex.getLoc();
   Init *DeclName = StringInit::get(Str);
   Lex.Lex();
 
-  if (ParsingTemplateArgs) {
-    if (CurRec)
-      DeclName = QualifyName(*CurRec, CurMultiClass, DeclName, ":");
-    else
-      assert(CurMultiClass);
-    if (CurMultiClass)
-      DeclName = QualifyName(CurMultiClass->Rec, CurMultiClass, DeclName,
-                             "::");
-  }
+  bool BadField;
+  if (!ParsingTemplateArgs) { // def, possibly in a multiclass
+    BadField = AddValue(CurRec, IdLoc,
+                        RecordVal(DeclName, IdLoc, Type,
+                                  HasField ? RecordVal::FK_NonconcreteOK
+                                           : RecordVal::FK_Normal));
+
+  } else if (CurRec) { // class template argument
+    BadField = AddValue(CurRec, IdLoc,
+                        RecordVal(DeclName, IdLoc, Type,
+                                  RecordVal::FK_TemplateArg));
 
-  // Add the field to the record.
-  if (AddValue(CurRec, IdLoc, RecordVal(DeclName, IdLoc, Type,
-                                        HasField ? RecordVal::FK_NonconcreteOK
-                                                 : RecordVal::FK_Normal)))
+  } else if (CurMultiClass) { // multiclass template argument
+    DeclName = QualifyName(CurMultiClass->Rec, CurMultiClass, DeclName, "::");
+    BadField = AddValue(CurRec, IdLoc,
+                        RecordVal(DeclName, IdLoc, Type,
+                                  RecordVal::FK_TemplateArg));
+  } else {
+    llvm_unreachable("invalid context for template argument");
+  }
+  if (BadField)
     return nullptr;
 
-  // If a value is present, parse it.
+  // If a value is present, parse it and set new field's value.
   if (consume(tgtok::equal)) {
     SMLoc ValLoc = Lex.getLoc();
     Init *Val = ParseValue(CurRec, Type);
     if (!Val ||
         SetValue(CurRec, ValLoc, DeclName, None, Val))
       // Return the name, even if an error is thrown.  This is so that we can
       // continue to make some progress, even without the value having been
       // initialized.
VarInit *TGParser::ParseForeachDeclaration(Init *&ForeachListValue) {
   if (!IterType)
     return nullptr;
 
   return VarInit::get(DeclName, IterType);
 }
 
 /// ParseTemplateArgList - Read a template argument list, which is a non-empty
 /// sequence of template-declarations in <>'s.  If CurRec is non-null, these are
-/// template args for a def, which may or may not be in a multiclass.  If null,
+/// template args for a class, which may or may not be in a multiclass. If null,
 /// these are the template args for a multiclass.
 ///
 ///    TemplateArgList ::= '<' Declaration (',' Declaration)* '>'
 ///
 bool TGParser::ParseTemplateArgList(Record *CurRec) {
   assert(Lex.getCode() == tgtok::less && "Not a template arg list!");
   Lex.Lex(); // eat the '<'
 

llvm/test/TableGen/self-reference-typeerror.td

 // RUN: not llvm-tblgen %s 2>&1 | FileCheck %s
 // XFAIL: vg_leak
 
 class A<A x> {
   A a = x;
 }
 
 // At the time A0 is referenced, A has not yet been established as a superclass.
 // This kind of self-reference is discourage, but if you *really* want it, you
 // can force it with !cast.
 //
-// CHECK: Field 'A:x' of type 'A' is incompatible with value
+// CHECK: nvalid value of type '{}' found when setting field 'a' of type 'A'
 def A0 : A<A0>;