[TableGen] Improve algorithms for processing template arguments; add type checking

[Paul-C-Anagnostopoulos]

Summary

The initial goal of this revision was to improve the algorithms for resolving the template arguments of classes and multiclasses. However, along the way I discovered that the type checking of template arguments was inconsistent.

• Template argument values passed to inherited classes were type-checked and cast if necessary.

• Template argument values to anonymous class instantiations were type-checked but not cast.

• Template argument values to multiclasses were not type-checked at all.

Adding type checking for multiclasses revealed many invalid template argument values. These have been corrected in the various target TableGen files. See https://reviews.llvm.org/D95874.

So this revision improves the algorithms and type-checks/casts all template arguments.

While this revision is being reviewed, I will add a large test for all the template argument checking.

[lattner]

I don't have technical competence to review the logic here (and, likely no one other than you do) but the code looks reasonable structurally.

Unrelated to the patch, you have looked into the vg_leak issues with tblgen tests?

[Paul-C-Anagnostopoulos]

@lattner Can you give me a link to a bug report or other discussion?

I also have to look into this bug: https://bugs.llvm.org/show_bug.cgi?id=48254. I don't feel particularly competent to investigate either one, but I will spend some time on them.

[lattner]

I just noticed this in the testcase: "// XFAIL: vg_leak". Valgrind checks for memory leaks, so I assume this is required because tblgen itself is leaking memory internally.

-Chris

[Paul-C-Anagnostopoulos]

Ah, I wondered what that was. 86 tests specify that XFAIL. When I can, I will spend the time to figure out how to track these down, though I remember something about the leak tracker-downers not working with Visual Studio.

[lattner]

If I recall it is more basic than that - lots of tblgen data structures don't have destructors or something. It has been forever since I've looked at this stuff so I don't recall. It might not be worth fixing

[Paul-C-Anagnostopoulos]

I cleaned up the tidies. I also made two simple changes to the TableGen Programmer's Reference.

[Paul-C-Anagnostopoulos]

I added a test for template argument checking and casting.

I had to change the invalid template argument type message to fatal. Otherwise all hell can break loose downstream.

[Paul-C-Anagnostopoulos]

@foad Something interesting is going on with the AMDGPU TableGen files. When I print all the definitions under main, there are 18,195 anonymous records. When I print them under these changes, there are 18,152 anonymous records. This suggests that fewer casts of records from one class to another are done under these changes. I'm not sure how that is possible. I will investigate further.

[foad]

In D96416#2568659, @Paul-C-Anagnostopoulos wrote:

@foad Something interesting is going on with the AMDGPU TableGen files. When I print all the definitions under main, there are 18,195 anonymous records. When I print them under these changes, there are 18,152 anonymous records. This suggests that fewer casts of records from one class to another are done under these changes. I'm not sure how that is possible. I will investigate further.

Interesting. Do the generated files lib/Target/AMDGPU/*.inc come out identical?

[Paul-C-Anagnostopoulos]

No, because the .inc files contain thousands of lines such as those below. I think it's rather strange that something like an enum has an enumerator with "anonymous_12703" in its name, but there might be good reasons for it.

enum {
  GIPFP_MI_Predicate_AMDGPUmul_i24_oneuse = GIPFP_MI_Invalid + 1,
  . . .
  GIPFP_MI_Predicate_and_oneuse,
  GIPFP_MI_Predicate_anonymous_12703,
  GIPFP_MI_Predicate_anonymous_12704,
  GIPFP_MI_Predicate_anonymous_12706,

[Paul-C-Anagnostopoulos]

I don't have any good ideas about how to approach this problem. However, while looking around, I'm finding things that confuse me even more. In AMDGPUSearchableTables.td:

class RsrcIntrinsic<AMDGPURsrcIntrinsic intr> {
  Intrinsic Intr = !cast<Intrinsic>(intr);
  bits<8> RsrcArg = intr.RsrcArg;
  bit IsImage = intr.IsImage;
}
. . .
foreach intr = !listconcat(AMDGPUBufferIntrinsics,
                         AMDGPUImageDimIntrinsics,
                         AMDGPUImageDimAtomicIntrinsics) in {
def : RsrcIntrinsic<!cast<AMDGPURsrcIntrinsic>(intr)>;

}

Notice that the class casts the 'intr' argument to type Intrinsic. But the lists that are concatenated in the foreach include AMDGPUBufferInstrinsics, which is a list of records of type AMDGPURsrcIntrinsic. That class is not a subclass of Intrinsic. So why aren't a pile of errors produced when trying to cast those record to type Intrinsic?

Ah, but now I see that one of the records in the AMDGPUBufferInstrinsics list is int_amdgcn_buffer_load_format, which is actually of type AMDGPUBufferLoad, which is a subclass of Instrinsic. Another record in the lits is int_amdgcn_s_buffer_load, which is of type Intrinsic. So probably all the record in the list are of type Intrinsic, which means there is no problem casting them.

This points out a problem with defset and/or !listconcat. I think it's !listconcat. But it probably has nothing to do with the issue at hand.

[Paul-C-Anagnostopoulos]

Okay, so the only trick I know to check the difference in anonymous record count is to take a copy of AMDGPUGenGlobalISel.inc generated under main and a copy generated under these changes and global replace all occurrences of 'anonymous_nnnnn' with 'xxx'. That normalizes them with respect to anonymous record names. Then I diff the two files. They are identical.

So I am going to somewhat rashly assume that the reduction in anonymous records is not a problem. I will push this revision on Thursday and stand by to see what happens. Meanwhile I will think about why this might happen.

[foad]

I'm seeing:

$ ninja -C ~/llvm-release/ check-llvm-codegen-amdgpu
ninja: Entering directory `/home/jayfoad2/llvm-release/'
[125/1115] Building RISCVGenAsmMatcher.inc...
FAILED: lib/Target/RISCV/RISCVGenAsmMatcher.inc 
cd /home/jayfoad2/llvm-release && /home/jayfoad2/llvm-release/bin/llvm-tblgen -gen-asm-matcher -I /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV -I/home/jayfoad2/llvm-release/include -I/home/jayfoad2/git/llvm-project/llvm/include -I /home/jayfoad2/git/llvm-project/llvm/lib/Target /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCV.td --write-if-changed -o lib/Target/RISCV/RISCVGenAsmMatcher.inc -d lib/Target/RISCV/RISCVGenAsmMatcher.inc.d
Included from /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCV.td:234:
Included from /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCVInstrInfo.td:1262:
Included from /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCVInstrInfoV.td:1166:
Included from /home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:4439:
/home/jayfoad2/git/llvm-project/llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:388:13: error: Value specified for template argument 'VPatUSLoadStoreWholeVRSDNode::type' (#0) is of type ValueType; expected type LLVMType: vti.Vector
  defm "" : VPatUSLoadStoreWholeVRSDNode<vti.Vector, vti.SEW, vti.LMul,
            ^
[150/1042] Building AMDGPUGenInstrInfo.inc...
ninja: build stopped: subcommand failed.

Could this be caused by your patch? Or maybe it's D96937?

[Paul-C-Anagnostopoulos]

Yes, that's due to this patch. As is this: http://lab.llvm.org:8011/#/builders/68/builds/7221/steps/4/logs/stdio

I've reverted the patch. I'll start with the RISCV problem and then ponder the Clang problem.

[Paul-C-Anagnostopoulos]

I am reopening this revision to pursue problems found when pushing the revision.

Please see the latest post here: https://reviews.llvm.org/D95874

[Paul-C-Anagnostopoulos]

@t.p.northover

I think the Clang problem might be in this code in arm_mve.td. I've flagged 'top', which is a bit. However, EmitterBase::getCodeForDagArg (in MveEmiiter.cpp) does not handle a bit as argument #2 of this DAG. At least, that's what I gather. I have improved the error message that it produces, which is shown below the code. What I don't understand is why this revision suddenly produces an error where none was produced before. I'll continue to investigate.

multiclass vmovl<bit top> {
  let params = [s8, u8, s16, u16] in {
    def "": Intrinsic<DblVector, (args Vector:$a),
      (extend (unzip $a, top), DblVector, (unsignedflag Scalar))>;
    defm "": IntrinsicMX<DblVector, (args Vector:$a, DblPredicate:$pred),
        (IRInt<"vmovl_predicated", [DblVector, Vector, DblPredicate]>
         $a, (unsignedflag Scalar), **top**, $pred, $inactive)>;
  }
}

error: bad DAG argument type for code generation
note: DAG: (anonymous_336 ?:$a, (unsignedflag Scalar), 0, ?:$pred, ?:$inactive)
note: argument type: bit
note: argument number 2: 0
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.

[Paul-C-Anagnostopoulos]

Ah, it's simple. 'top' is a template argument to the vmovl multiclass, with type bit. That multiclass is invoked:

defm vmovlbq: vmovl<0>;
defm vmovltq: vmovl<1>;

Before this revision, the code didn't check that 0 and 1 were integers, so they passed through to the DAG as integers, which is what EmitterBase::getCodeForDagArg wants. Now they are checked and cast to bits, which getCodeForDagArg does not handle.

Note that multiclass vshll_imm also has a top argument, but it is declared int.

[Paul-C-Anagnostopoulos]

Since I haven't heard a peep from anyone wanting to fix arm_mve.td, I'll try to fix it myself.

(I've been incommunicado for a week while moving to my new machine.)

[Paul-C-Anagnostopoulos]

Okay, I've fixed the arm_mve.td template argument problem with a change to MveEmitter.cpp. I will update this revision, let it cook for a couple of days, and then try pushing it again.

[Paul-C-Anagnostopoulos]

This update fixes the arm_mve.td problem with a change to MveEmitter.cpp.

[RKSimon]

@Paul-C-Anagnostopoulos Whats the status of this now?

[Paul-C-Anagnostopoulos]

I will push this again on Thursday. I expect it to fail with some new and interesting issue, so I need to be around. I have been in and out too much recently.

[Paul-C-Anagnostopoulos]

Sorry, but I have to wait until Friday to push this again.

[Paul-C-Anagnostopoulos]

Okay, I will push this revision within the next half hour.

llvm/docs/TableGen/ProgRef.rst

``dag``
     another ``dag`` object, allowing an arbitrary tree of nodes and edges.
     As an example, DAGs are used to represent code patterns for use by
     the code generator instruction selection algorithms. See `Directed
     acyclic graphs (DAGs)`_ for more details;
 
 :token:`ClassID`
     Specifying a class name in a type context indicates
     that the type of the defined value must
-    be a subclass of the specified class.  This is useful in conjunction with
+    be a subclass of the specified class. This is useful in conjunction with
     the ``list`` type; for example, to constrain the elements of the list to a
     common base class (e.g., a ``list<Register>`` can only contain definitions
     derived from the ``Register`` class).
     The :token:`ClassID` must name a class that has been previously
     declared or defined.
 
 
 Values and Expressions

 classes and records can inherit. 
 
 .. productionlist::
    Class: "class" `ClassID` [`TemplateArgList`] `RecordBody`
    TemplateArgList: "<" `TemplateArgDecl` ("," `TemplateArgDecl`)* ">"
    TemplateArgDecl: `Type` `TokIdentifier` ["=" `Value`]
 
 A class can be parameterized by a list of "template arguments," whose values
-can be used in the class's record body.  These template arguments are
+can be used in the class's record body. These template arguments are
 specified each time the class is inherited by another class or record.
 
 If a template argument is not assigned a default value with ``=``, it is
 uninitialized (has the "value" ``?``) and must be specified in the template
-argument list when the class is inherited. If an argument is assigned a
-default value, then it need not be specified in the argument list. The
-template argument default values are evaluated from left to right.
+argument list when the class is inherited (required argument). If an
+argument is assigned a default value, then it need not be specified in the
+argument list (optional argument). In the declaration, all required template
+arguments must precede any optional arguments. The template argument default
+values are evaluated from left to right.
 
 The :token:`RecordBody` is defined below. It can include a list of
-superclasses from which the current class inherits, along with field definitions
-and other statements. When a class ``C`` inherits from another class ``D``,
-the fields of ``D`` are effectively merged into the fields of ``C``.
+superclasses from which the current class inherits, along with field
+definitions and other statements. When a class ``C`` inherits from another
+class ``D``, the fields of ``D`` are effectively merged into the fields of
+``C``.
 
 A given class can only be defined once. A ``class`` statement is
 considered to define the class if *any* of the following are true (the
 :token:`RecordBody` elements are described below).
 
 * The :token:`TemplateArgList` is present, or
 * The :token:`ParentClassList` in the :token:`RecordBody` is present, or
 * The :token:`Body` in the :token:`RecordBody` is present and not empty.

 superclasses or parent classes of the class or record. The record body also
 includes the main body of the definition, which contains the specification
 of the fields of the class or record.
 
 .. productionlist::
    RecordBody: `ParentClassList` `Body`
    ParentClassList: [":" `ParentClassListNE`]
    ParentClassListNE: `ClassRef` ("," `ClassRef`)*
-   ClassRef: (`ClassID` | `MultiClassID`) ["<" `ValueList` ">"]
+   ClassRef: (`ClassID` | `MultiClassID`) ["<" [`ValueList`] ">"]
 
 A :token:`ParentClassList` containing a :token:`MultiClassID` is valid only
 in the class list of a ``defm`` statement. In that case, the ID must be the
 name of a multiclass.
 
 .. productionlist::
    Body: ";" | "{" `BodyItem`* "}"
    BodyItem: (`Type` | "code") `TokIdentifier` ["=" `Value`] ";"

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();
+  }
+
   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;
   Init *Name = nullptr;

llvm/lib/TableGen/Record.cpp

if (SkipVal == &Value) // Skip resolve the same field as the given one
       continue;
     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 '" +
-                                      Value.getNameInitAsString() +
-                                      "' of type '" +
-                                      Value.getType()->getAsString() +
-                                      "' after resolving references: " +
-                                      VR->getAsUnquotedString() + "\n");
+        PrintFatalError(
+            getLoc(),
+            Twine("Invalid value ") + Type + "found when setting field '" +
+                Value.getNameInitAsString() + "' of type '" +
+                "' after resolving references: " + VR->getAsUnquotedString() +
+                "\n");
       }
     }
   }
 }
 
 void Record::resolveReferences(Init *NewName) {
   RecordResolver R(*this);
   R.setName(NewName);

llvm/lib/TableGen/TGParser.h

private:  // Parser methods.
   SubMultiClassReference ParseSubMultiClassReference(MultiClass *CurMC);
 
   Init *ParseIDValue(Record *CurRec, StringInit *Name, SMLoc NameLoc,
                      IDParseMode Mode = ParseValueMode);
   Init *ParseSimpleValue(Record *CurRec, RecTy *ItemType = nullptr,
                          IDParseMode Mode = ParseValueMode);
   Init *ParseValue(Record *CurRec, RecTy *ItemType = nullptr,
                    IDParseMode Mode = ParseValueMode);
-  void ParseValueList(SmallVectorImpl<llvm::Init*> &Result, Record *CurRec,
-                      Record *ArgsRec = nullptr, RecTy *EltTy = nullptr);
+  void ParseValueList(SmallVectorImpl<llvm::Init*> &Result,
+                      Record *CurRec, RecTy *ItemType = nullptr);
+  bool ParseTemplateArgValueList(SmallVectorImpl<llvm::Init *> &Result,
+                                 Record *CurRec, Record *ArgsRec);
   void ParseDagArgList(
       SmallVectorImpl<std::pair<llvm::Init*, StringInit*>> &Result,
       Record *CurRec);
   bool ParseOptionalRangeList(SmallVectorImpl<unsigned> &Ranges);
   bool ParseOptionalBitList(SmallVectorImpl<unsigned> &Ranges);
   void ParseRangeList(SmallVectorImpl<unsigned> &Result);
   bool ParseRangePiece(SmallVectorImpl<unsigned> &Ranges,
                        TypedInit *FirstItem = nullptr);
   RecTy *ParseType();
   Init *ParseOperation(Record *CurRec, RecTy *ItemType);
   Init *ParseOperationSubstr(Record *CurRec, RecTy *ItemType);
   Init *ParseOperationForEachFilter(Record *CurRec, RecTy *ItemType);
   Init *ParseOperationCond(Record *CurRec, RecTy *ItemType);
   RecTy *ParseOperatorType();
   Init *ParseObjectName(MultiClass *CurMultiClass);
   Record *ParseClassID();
   MultiClass *ParseMultiClassID();
   bool ApplyLetStack(Record *CurRec);
   bool ApplyLetStack(RecordsEntry &Entry);
+  bool CheckTemplateArgValues(SmallVectorImpl<llvm::Init *> &Values,
+                              SMLoc Loc, Record *ArgsRec);
   void CheckAssert(SMLoc Loc, Init *Condition, Init *Message);
   void CheckRecordAsserts(Record &Rec);
 };
 
 } // end namespace llvm
 
 #endif

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 '" +
+                       TArgs[I]->getAsUnquotedString() + "' (#" + Twine(I) +
+                       ") of parent class '" + SC->getNameInitAsString() + "'");
   }
 
   Init *Name;
   if (CurRec->isClass())
     Name =
         VarInit::get(QualifiedNameOfImplicitName(*CurRec), StringRecTy::get());
   else
     Name = CurRec->getNameInit();
MultiClass *TGParser::ParseMultiClassID() {
   MultiClass *Result = MultiClasses[Lex.getCurStrVal()].get();
   if (!Result)
     TokError("Couldn't find multiclass '" + Lex.getCurStrVal() + "'");
 
   Lex.Lex();
   return Result;
 }
 
-/// ParseSubClassReference - Parse a reference to a subclass or to a templated
-/// subclass.  This returns a SubClassRefTy with a null Record* on error.
+/// ParseSubClassReference - Parse a reference to a subclass or a
+/// multiclass. This returns a SubClassRefTy with a null Record* on error.
 ///
 ///  SubClassRef ::= ClassID
 ///  SubClassRef ::= ClassID '<' ValueList '>'
 ///
 SubClassReference TGParser::
 ParseSubClassReference(Record *CurRec, bool isDefm) {
   SubClassReference Result;
   Result.RefRange.Start = Lex.getLoc();
 
   if (isDefm) {
     if (MultiClass *MC = ParseMultiClassID())
       Result.Rec = &MC->Rec;
   } else {
     Result.Rec = ParseClassID();
   }
   if (!Result.Rec) return Result;
 
   // If there is no template arg list, we're done.
   if (!consume(tgtok::less)) {
     Result.RefRange.End = Lex.getLoc();
     return Result;
   }
 
-  if (Lex.getCode() == tgtok::greater) {
-    TokError("subclass reference requires a non-empty list of template values");
-    Result.Rec = nullptr;
-    return Result;
-  }
-
-  ParseValueList(Result.TemplateArgs, CurRec, Result.Rec);
-  if (Result.TemplateArgs.empty()) {
-    Result.Rec = nullptr;   // Error parsing value list.
+  if (ParseTemplateArgValueList(Result.TemplateArgs, CurRec, Result.Rec)) {
+    Result.Rec = nullptr; // Error parsing value list.
     return Result;
   }
 
-  if (!consume(tgtok::greater)) {
-    TokError("expected '>' in template value list");
-    Result.Rec = nullptr;
+  if (CheckTemplateArgValues(Result.TemplateArgs, Result.RefRange.Start,
+                             Result.Rec)) {
+    Result.Rec = nullptr; // Error checking value list.
     return Result;
   }
-  Result.RefRange.End = Lex.getLoc();
 
+  Result.RefRange.End = Lex.getLoc();
   return Result;
 }
 
 /// ParseSubMultiClassReference - Parse a reference to a subclass or to a
 /// templated submulticlass.  This returns a SubMultiClassRefTy with a null
 /// Record* on error.
 ///
 ///  SubMultiClassRef ::= MultiClassID
 ///  SubMultiClassRef ::= MultiClassID '<' ValueList '>'
 ///
 SubMultiClassReference TGParser::
 ParseSubMultiClassReference(MultiClass *CurMC) {
   SubMultiClassReference Result;
   Result.RefRange.Start = Lex.getLoc();
 
   Result.MC = ParseMultiClassID();
   if (!Result.MC) return Result;
 
   // If there is no template arg list, we're done.
   if (!consume(tgtok::less)) {
     Result.RefRange.End = Lex.getLoc();
     return Result;
   }
 
-  if (Lex.getCode() == tgtok::greater) {
-    TokError("subclass reference requires a non-empty list of template values");
-    Result.MC = nullptr;
-    return Result;
-  }
-
-  ParseValueList(Result.TemplateArgs, &CurMC->Rec, &Result.MC->Rec);
-  if (Result.TemplateArgs.empty()) {
-    Result.MC = nullptr;   // Error parsing value list.
+  if (ParseTemplateArgValueList(Result.TemplateArgs, &CurMC->Rec,
+                                &Result.MC->Rec)) {
+    Result.MC = nullptr; // Error parsing value list.
     return Result;
   }
 
-  if (!consume(tgtok::greater)) {
-    TokError("expected '>' in template value list");
-    Result.MC = nullptr;
-    return Result;
-  }
   Result.RefRange.End = Lex.getLoc();
 
   return Result;
 }
 
 /// ParseRangePiece - Parse a bit/value range.
 ///   RangePiece ::= INTVAL
 ///   RangePiece ::= INTVAL '...' INTVAL
case tgtok::question:
     Lex.Lex();
     break;
   case tgtok::Id: {
     SMLoc NameLoc = Lex.getLoc();
     StringInit *Name = StringInit::get(Lex.getCurStrVal());
     if (Lex.Lex() != tgtok::less)  // consume the Id.
       return ParseIDValue(CurRec, Name, NameLoc, Mode);    // Value ::= IDValue
 
-    // Value ::= ID '<' ValueListNE '>'
-    if (Lex.Lex() == tgtok::greater) {
-      TokError("expected non-empty value list");
-      return nullptr;
-    }
-
-    // This is a CLASS<initvalslist> expression.  This is supposed to synthesize
-    // a new anonymous definition, deriving from CLASS<initvalslist> with no
-    // body.
+    // Value ::= CLASSID '<' ValueListNE '>' (CLASSID has been consumed)
+    // This is supposed to synthesize a new anonymous definition, deriving
+    // from the class with the template arguments, but no body.
     Record *Class = Records.getClass(Name->getValue());
     if (!Class) {
       Error(NameLoc, "Expected a class name, got '" + Name->getValue() + "'");
       return nullptr;
     }
 
     SmallVector<Init *, 8> Args;
-    ParseValueList(Args, CurRec, Class);
-    if (Args.empty()) return nullptr;
+    Lex.Lex(); // consume the <
+    if (ParseTemplateArgValueList(Args, CurRec, Class))
+      return nullptr; // Error parsing value list.
+
+    if (CheckTemplateArgValues(Args, NameLoc, Class))
+      return nullptr; // Error checking template argument values.
+
+    // Loop through the arguments that were not specified and make sure
+    // they have a complete value.
+    // TODO: If we just keep a required argument count, we can do away
+    //       with this checking.
+    ArrayRef<Init *> TArgs = Class->getTemplateArgs();
+    for (unsigned I = Args.size(), E = TArgs.size(); I < E; ++I) {
+      RecordVal *Arg = Class->getValue(TArgs[I]);
+      if (!Arg->getValue()->isComplete()) 
+        Error(NameLoc, "Value not specified for template argument '" +
+                           TArgs[I]->getAsUnquotedString() + "' (#" + Twine(I) +
+                           ") of parent class '" +
+                           Class->getNameInitAsString() + "'");
               
-    if (!consume(tgtok::greater)) {
-      TokError("expected '>' at end of value list");
-      return nullptr;
-    }
-
-    // Typecheck the template arguments list
-    ArrayRef<Init *> ExpectedArgs = Class->getTemplateArgs();
-    if (ExpectedArgs.size() < Args.size()) {
-      Error(NameLoc,
-            "More template args specified than expected");
-      return nullptr;
-    }
-
-    for (unsigned i = 0, e = ExpectedArgs.size(); i != e; ++i) {
-      RecordVal *ExpectedArg = Class->getValue(ExpectedArgs[i]);
-      if (i < Args.size()) {
-        if (TypedInit *TI = dyn_cast<TypedInit>(Args[i])) {
-          RecTy *ExpectedType = ExpectedArg->getType();
-          if (!TI->getType()->typeIsConvertibleTo(ExpectedType)) {
-            Error(NameLoc,
-                  "Value specified for template argument #" + Twine(i) + " (" +
-                  ExpectedArg->getNameInitAsString() + ") is of type '" +
-                  TI->getType()->getAsString() + "', expected '" +
-                  ExpectedType->getAsString() + "': " + TI->getAsString());
-            return nullptr;
-          }
-          continue;
-        }
-      } else if (ExpectedArg->getValue()->isComplete())
-        continue;
-
-      Error(NameLoc,
-            "Value not specified for template argument #" + Twine(i) + " (" +
-            ExpectedArgs[i]->getAsUnquotedString() + ")");
-      return nullptr;
     }
 
     return VarDefInit::get(Class, Args)->Fold();
   }
   case tgtok::l_brace: {           // Value ::= '{' ValueList '}'
     SMLoc BraceLoc = Lex.getLoc();
     Lex.Lex(); // eat the '{'
     SmallVector<Init*, 16> Vals;
if (ItemType) {
         TokError(Twine("Encountered a list when expecting a ") +
                  ItemType->getAsString());
         return nullptr;
       }
       GivenListTy = ListType;
     }
 
     if (Lex.getCode() != tgtok::r_square) {
-      ParseValueList(Vals, CurRec, nullptr,
+      ParseValueList(Vals, CurRec,
                      GivenListTy ? GivenListTy->getElementType() : nullptr);
       if (Vals.empty()) return nullptr;
     }
     if (!consume(tgtok::r_square)) {
       TokError("expected ']' at end of list value");
       return nullptr;
     }
 
if (Lex.getCode() == tgtok::VarName) {
 
       Result.push_back(std::make_pair(Val, VarName));
     }
     if (!consume(tgtok::comma))
       break;
   }
 }
 
-/// ParseValueList - Parse a comma separated list of values, returning them as a
-/// vector.  Note that this always expects to be able to parse at least one
-/// value.  It returns an empty list if this is not possible.
+/// ParseValueList - Parse a comma separated list of values, returning them
+/// in a vector. Note that this always expects to be able to parse at least one
+/// value. It returns an empty list if this is not possible.
 ///
 ///   ValueList ::= Value (',' Value)
 ///
-void TGParser::ParseValueList(SmallVectorImpl<Init*> &Result, Record *CurRec,
-                              Record *ArgsRec, RecTy *EltTy) {
-  RecTy *ItemType = EltTy;
-  unsigned int ArgN = 0;
-  if (ArgsRec && !EltTy) {
-    ArrayRef<Init *> TArgs = ArgsRec->getTemplateArgs();
-    if (TArgs.empty()) {
-      TokError("template argument provided to non-template class");
-      Result.clear();
-      return;
-    }
-    const RecordVal *RV = ArgsRec->getValue(TArgs[ArgN]);
-    if (!RV) {
-      errs() << "Cannot find template arg " << ArgN << " (" << TArgs[ArgN]
-        << ")\n";
-    }
-    assert(RV && "Template argument record not found??");
-    ItemType = RV->getType();
-    ++ArgN;
-  }
+void TGParser::ParseValueList(SmallVectorImpl<Init *> &Result, Record *CurRec,
+                              RecTy *ItemType) {
+
   Result.push_back(ParseValue(CurRec, ItemType));
   if (!Result.back()) {
     Result.clear();
     return;
   }
 
   while (consume(tgtok::comma)) {
     // ignore trailing comma for lists
     if (Lex.getCode() == tgtok::r_square)
       return;
-
-    if (ArgsRec && !EltTy) {
-      ArrayRef<Init *> TArgs = ArgsRec->getTemplateArgs();
-      if (ArgN >= TArgs.size()) {
-        TokError("too many template arguments");
-        Result.clear();
-        return;
-      }
-      const RecordVal *RV = ArgsRec->getValue(TArgs[ArgN]);
-      assert(RV && "Template argument record not found??");
-      ItemType = RV->getType();
-      ++ArgN;
-    }
     Result.push_back(ParseValue(CurRec, ItemType));
     if (!Result.back()) {
       Result.clear();
       return;
     }
   }
 }
 
+// ParseTemplateArgValueList - Parse a template argument list with the syntax
+// shown, filling in the Result vector. The open angle has been consumed.
+// An empty argument list is allowed. Return false if okay, true if an 
+// error was detected.
+//
+//   TemplateArgList ::= '<' [Value {',' Value}*] '>'
+bool TGParser::ParseTemplateArgValueList(SmallVectorImpl<Init *> &Result,
+                                         Record *CurRec, Record *ArgsRec) {
+
+  assert(Result.empty() && "Result vector is not empty");
+  ArrayRef<Init *> TArgs = ArgsRec->getTemplateArgs();
+  unsigned ArgIndex = 0;
+  RecTy *ItemType;
+
+  if (consume(tgtok::greater)) // empty value list
+    return false;
+
+  while (true) {
+    if (ArgIndex >= TArgs.size()) {
+      TokError("Too many template arguments: " + utostr(ArgIndex + 1));
+      return true;
+    }
+    const RecordVal *Arg = ArgsRec->getValue(TArgs[ArgIndex]);
+    assert(Arg && "Template argument record not found");
+
+    ItemType = Arg->getType();
+    Init *Value = ParseValue(CurRec, ItemType);
+    if (!Value)
+      return true;
+    Result.push_back(Value);
+
+    if (consume(tgtok::greater)) // end of argument list?
+      return false;
+    if (!consume(tgtok::comma)) // must be comma
+      return true;
+    ++ArgIndex; 
+  }
+}
+
 /// 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 { // multiclass template argument
+    assert(CurMultiClass && "invalid context for template argument");
+    DeclName = QualifyName(CurMultiClass->Rec, CurMultiClass, DeclName, "::");
+    BadField = AddValue(CurRec, IdLoc, RecordVal(DeclName, IdLoc, Type,
+                                                 RecordVal::FK_TemplateArg));
+  }
+  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.
default: {
     return nullptr;
   }
   }
 
 
   if (!Ranges.empty()) {
     assert(!IterType && "Type already initialized?");
     IterType = IntRecTy::get();
-    std::vector<Init*> Values;
+    std::vector<Init *> Values;
     for (unsigned R : Ranges)
       Values.push_back(IntInit::get(R));
     ForeachListValue = ListInit::get(Values, IterType);
   }
 
   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 '<'
 
bool TGParser::ParseDefm(MultiClass *CurMultiClass) {
   Lex.Lex();
 
   SMLoc SubClassLoc = Lex.getLoc();
   SubClassReference Ref = ParseSubClassReference(nullptr, true);
 
   while (true) {
     if (!Ref.Rec) return true;
 
-    // To instantiate a multiclass, we need to first get the multiclass, then
-    // instantiate each def contained in the multiclass with the SubClassRef
-    // template parameters.
+    // To instantiate a multiclass, we get the multiclass and then loop
+    // through its template argument names. Substs contains a substitution
+    // value for each argument, either the value specified or the default.
+    // Then we can resolve the template arguments.
     MultiClass *MC = MultiClasses[std::string(Ref.Rec->getName())].get();
     assert(MC && "Didn't lookup multiclass correctly?");
-    ArrayRef<Init*> TemplateVals = Ref.TemplateArgs;
 
-    // Verify that the correct number of template arguments were specified.
+    ArrayRef<Init *> TemplateVals = Ref.TemplateArgs;
     ArrayRef<Init *> TArgs = MC->Rec.getTemplateArgs();
-    if (TArgs.size() < TemplateVals.size())
-      return Error(SubClassLoc,
-                   "more template args specified than multiclass expects");
-
     SubstStack Substs;
+
     for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
       if (i < TemplateVals.size()) {
         Substs.emplace_back(TArgs[i], TemplateVals[i]);
       } else {
         Init *Default = MC->Rec.getValue(TArgs[i])->getValue();
-        if (!Default->isComplete()) {
+        if (!Default->isComplete())
           return Error(SubClassLoc,
-                       "value not specified for template argument #" +
-                           Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
-                           ") of multiclass '" + MC->Rec.getNameInitAsString() +
-                           "'");
-        }
+                       "value not specified for template argument '" +
+                           TArgs[i]->getAsUnquotedString() + "' (#" +
+                           Twine(i) + ") of multiclass '" +
+                           MC->Rec.getNameInitAsString() + "'");
         Substs.emplace_back(TArgs[i], Default);
       }
     }
 
     Substs.emplace_back(QualifiedNameOfImplicitName(MC), DefmName);
 
     if (resolve(MC->Entries, Substs, !CurMultiClass && Loops.empty(),
                 &NewEntries, &SubClassLoc))
       return true;
 
     if (!consume(tgtok::comma))
       break;
 
     if (Lex.getCode() != tgtok::Id)
       return TokError("expected identifier");
 
     SubClassLoc = Lex.getLoc();
 
-    // A defm can inherit from regular classes (non-multiclass) as
+    // A defm can inherit from regular classes (non-multiclasses) as
     // long as they come in the end of the inheritance list.
     InheritFromClass = (Records.getClass(Lex.getCurStrVal()) != nullptr);
 
     if (InheritFromClass)
       break;
 
     Ref = ParseSubClassReference(nullptr, true);
   }
bool TGParser::ParseFile() {
 
   // If we have unread input at the end of the file, report it.
   if (Lex.getCode() == tgtok::Eof)
     return false;
 
   return TokError("Unexpected token at top level");
 }
 
+// Check the types of the template argument values for a class
+// inheritance, multiclass invocation, or anonymous class invocation.
+// If necessary, replace an argument with a cast to the required type.
+// The argument count has already been checked.
+bool TGParser::CheckTemplateArgValues(SmallVectorImpl<llvm::Init *> &Values,
+                                      SMLoc Loc, Record *ArgsRec) {
+
+  ArrayRef<Init *> TArgs = ArgsRec->getTemplateArgs();
+
+  for (unsigned I = 0, E = Values.size(); I < E; ++I) {
+    RecordVal *Arg = ArgsRec->getValue(TArgs[I]);
+    RecTy *ArgType = Arg->getType();
+    auto *Value = Values[I];
+
+    if (TypedInit *ArgValue = dyn_cast<TypedInit>(Value)) { 
+      auto *CastValue = ArgValue->getCastTo(ArgType);
+      if (CastValue) {
+        assert((!isa<TypedInit>(CastValue) ||
+                cast<TypedInit>(CastValue)->getType()->typeIsA(ArgType)) &&
+               "result of template arg value cast has wrong type");
+        Values[I] = CastValue;
+      } else {
+        PrintError(Loc, "Value specified for template argument '" +
+                            Arg->getNameInitAsString() + "' (#" + Twine(I) +
+                            ") is of type " + ArgValue->getType()->getAsString() +
+                            "; expected type " + ArgType->getAsString() + ": " +
+                            ArgValue->getAsString());
+      }
+    }
+  }
+
+  return false;
+}
+
 // Check an assertion: Obtain the condition value and be sure it is true.
 // If not, print a nonfatal error along with the message.
 void TGParser::CheckAssert(SMLoc Loc, Init *Condition, Init *Message) {
   auto *CondValue = dyn_cast_or_null<IntInit>(
                         Condition->convertInitializerTo(IntRecTy::get()));
   if (CondValue) {
     if (!CondValue->getValue()) {
       PrintError(Loc, "assertion failed");

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;
+class Cl<Cl rec> {
+  Cl Arec = rec;
 }
 
 // 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
-def A0 : A<A0>;
+// CHECK: alue specified for template argument 'Cl:rec'
+
+def Rec0 : Cl<Rec0>;