TableGen: Streamline how defs are instantiated

[nhaehnle]

Summary

Instantiating def's and defm's needs to perform the following steps:

• for defm's, clone multiclass def prototypes and subsitute template args
• for def's and defm's, add subclass definitions, substituting template args
• clone the record based on foreach loops and substitute loop iteration variables
• override record variables based on the global 'let' stack
• resolve the record name (this should be simple, but unfortunately it's not due to existing .td files relying on rather silly implementation details)
• for def(m)s in multiclasses, add the unresolved record as a multiclass prototype
• for top-level def(m)s, resolve all internal variable references and add them to the record keeper and any active defsets

This change streamlines how we go through these steps, by having both
def's and defm's feed into a single addDef() method that handles foreach,
final resolve, and routing the record to the right place.

This happens to make foreach inside of multiclasses work, as the new
test case demonstrates. Previously, foreach inside multiclasses was not
forbidden by the parser, but it was de facto broken.

Another side effect is that the order of "instantiated from" notes in error
messages is reversed, as the modified test case shows. This is arguably
clearer, since the initial error message ends up pointing directly to
whatever triggered the error, and subsequent notes will point to increasingly
outer layers of multiclasses. This is consistent with how C++ compilers
report nested #includes and nested template instantiations.

Change-Id: Ica146d0db2bc133dd7ed88054371becf24320447

[nhaehnle]

Updating with a cleanup that I missed.

[tra]

Just in time. :-) I've ran into the broken foreach-in-multiclass yesterday.

lib/TableGen/TGParser.cpp

2453 ↗

(On Diff #138371)

Nit: /*Anonymous=*/true.

2798 ↗

(On Diff #138371)

Number of items in a multiclass is relatively small. Perhaps we could/should use SmallVector here.

2853–2856 ↗

(On Diff #138371)

It would be good to have an example or a pointer to something specific so whoever gets to read it later has an ises what needs to be fixed. Perhaps it would make sense to add a disabled test case that would demonstrate the issue.

2867–2868 ↗

(On Diff #138371)

Same here.

[nhaehnle]

Address minor review comments and add a new test case that demonstrates
the inconsistencies in name resolution.

[nhaehnle]

lib/TableGen/TGParser.cpp

2453 ↗

(On Diff #138371)

Done.

2798 ↗

(On Diff #138371)

Done.

2853–2856 ↗

(On Diff #138371)

Yeah, that's absolutely true.

I added a new test case that shows some of the surprising stuff that happens, in particular: the resolution of record names referencing template args depends on the nesting level at which the template arg is substituted, and trying to add prefixes inside multiclasses ends up with some surprises as well.

llvm/trunk/include/llvm/TableGen/Record.h

public:
   // Constructs a record.
   explicit Record(Init *N, ArrayRef<SMLoc> locs, RecordKeeper &records,
                   bool Anonymous = false) :
     Name(N), Locs(locs.begin(), locs.end()), TrackedRecords(records),
     ID(LastID++), IsAnonymous(Anonymous) {
     init();
   }
 
-  explicit Record(StringRef N, ArrayRef<SMLoc> locs, RecordKeeper &records,
-                  bool Anonymous = false)
-    : Record(StringInit::get(N), locs, records, Anonymous) {}
+  explicit Record(StringRef N, ArrayRef<SMLoc> locs, RecordKeeper &records)
+      : Record(StringInit::get(N), locs, records) {}
 
   // When copy-constructing a Record, we must still guarantee a globally unique
   // ID number.  Don't copy TheInit either since it's owned by the original
   // record. All other fields can be copied normally.
   Record(const Record &O) :
     Name(O.Name), Locs(O.Locs), TemplateArgs(O.TemplateArgs),
     Values(O.Values), SuperClasses(O.SuperClasses),
     TrackedRecords(O.TrackedRecords), ID(LastID++),
public:
 
   const std::string getNameInitAsString() const {
     return getNameInit()->getAsUnquotedString();
   }
 
   void setName(Init *Name);      // Also updates RecordKeeper.
 
   ArrayRef<SMLoc> getLoc() const { return Locs; }
+  void appendLoc(SMLoc Loc) { Locs.push_back(Loc); }
+
+  // Make the type that this record should have based on its superclasses.
+  RecordRecTy *getType();
 
   /// get the corresponding DefInit.
   DefInit *getDefInit();
 
   ArrayRef<Init *> getTemplateArgs() const {
     return TemplateArgs;
   }
 
for (const auto &SCPair : SuperClasses) {
       } else if (SCPair.first->getNameInitAsString() == Name) {
         return true;
       }
     }
     return false;
   }
 
   void addSuperClass(Record *R, SMRange Range) {
+    assert(!TheInit && "changing type of record after it has been referenced");
     assert(!isSubClassOf(R) && "Already subclassing record!");
     SuperClasses.push_back(std::make_pair(R, Range));
   }
 
   /// If there are any field references that refer to fields
   /// that have been filled in, we can propagate the values now.
   ///
   /// This is a final resolve: any error messages, e.g. due to undefined

llvm/trunk/lib/TableGen/Record.cpp

 }
 
 Init *VarListElementInit::getBit(unsigned Bit) const {
   if (getType() == BitRecTy::get())
     return const_cast<VarListElementInit*>(this);
   return VarBitInit::get(const_cast<VarListElementInit*>(this), Bit);
 }
 
-static RecordRecTy *makeDefInitType(Record *Rec) {
-  SmallVector<Record *, 4> SuperClasses;
-  Rec->getDirectSuperClasses(SuperClasses);
-  return RecordRecTy::get(SuperClasses);
-}
-
 DefInit::DefInit(Record *D)
-    : TypedInit(IK_DefInit, makeDefInitType(D)), Def(D) {}
+    : TypedInit(IK_DefInit, D->getType()), Def(D) {}
 
 DefInit *DefInit::get(Record *R) {
   return R->getDefInit();
 }
 
 Init *DefInit::convertInitializerTo(RecTy *Ty) const {
   if (auto *RRT = dyn_cast<RecordRecTy>(Ty))
     if (getType()->typeIsConvertibleTo(RRT))
void Record::init() {
   // replaced with the top-level def name at instantiation time.
   addValue(RecordVal(StringInit::get("NAME"), StringRecTy::get(), false));
 }
 
 void Record::checkName() {
   // Ensure the record name has string type.
   const TypedInit *TypedName = cast<const TypedInit>(Name);
   if (!isa<StringRecTy>(TypedName->getType()))
-    PrintFatalError(getLoc(), "Record name is not a string!");
+    PrintFatalError(getLoc(), Twine("Record name '") + Name->getAsString() +
+                                  "' is not a string!");
+}
+
+RecordRecTy *Record::getType() {
+  SmallVector<Record *, 4> DirectSCs;
+  getDirectSuperClasses(DirectSCs);
+  return RecordRecTy::get(DirectSCs);
 }
 
 DefInit *Record::getDefInit() {
   if (!TheInit)
     TheInit = new(Allocator) DefInit(this);
   return TheInit;
 }
 

llvm/trunk/lib/TableGen/TGParser.h

struct IterRecord {
     Init *IterValue;
     IterRecord(VarInit *Var, Init *Val) : IterVar(Var), IterValue(Val) {}
   };
 
   // IterSet: The set of all iterator values at some point in the
   // iteration space.
   typedef std::vector<IterRecord> IterSet;
 
-  bool ProcessForeachDefs(Record *CurRec, SMLoc Loc);
-  bool ProcessForeachDefs(Record *CurRec, SMLoc Loc, IterSet &IterVals);
-
-  bool addToDefsets(Record &R);
+  bool addDefOne(std::unique_ptr<Record> Rec, Init *DefmName,
+                 IterSet &IterVals);
+  bool addDefForeach(Record *Rec, Init *DefmName, IterSet &IterVals);
+  bool addDef(std::unique_ptr<Record> Rec, Init *DefmName);
 
 private:  // Parser methods.
   bool ParseObjectList(MultiClass *MC = nullptr);
   bool ParseObject(MultiClass *MC);
   bool ParseClass();
   bool ParseMultiClass();
-  Record *InstantiateMulticlassDef(MultiClass &MC, Record *DefProto,
-                                   Init *&DefmPrefix, SMRange DefmPrefixRange,
-                                   ArrayRef<Init *> TArgs,
-                                   ArrayRef<Init *> TemplateVals);
-  bool ResolveMulticlassDefArgs(MultiClass &MC, Record *DefProto,
-                                SMLoc DefmPrefixLoc, SMLoc SubClassLoc,
-                                ArrayRef<Init *> TArgs,
-                                ArrayRef<Init *> TemplateVals, bool DeleteArgs);
-  bool ResolveMulticlassDef(MultiClass &MC,
-                            Record *CurRec,
-                            Record *DefProto,
-                            SMLoc DefmPrefixLoc);
   bool ParseDefm(MultiClass *CurMultiClass);
   bool ParseDef(MultiClass *CurMultiClass);
   bool ParseDefset();
   bool ParseForeach(MultiClass *CurMultiClass);
   bool ParseTopLevelLet(MultiClass *CurMultiClass);
   void ParseLetList(SmallVectorImpl<LetRecord> &Result);
 
   bool ParseObjectBody(Record *CurRec);

llvm/trunk/lib/TableGen/TGParser.cpp

for (const auto &Def :
     }
 
     Def->resolveReferences(R);
   }
 
   return false;
 }
 
-/// ProcessForeachDefs - Given a record, apply all of the variable
-/// values in all surrounding foreach loops, creating new records for
-/// each combination of values.
-bool TGParser::ProcessForeachDefs(Record *CurRec, SMLoc Loc) {
+/// Add a record that results from 'def' or 'defm', after template arguments
+/// and the external let stack have been resolved.
+///
+/// Apply foreach loops, resolve internal variable references, and add to the
+/// current multi class or the global record keeper as appropriate.
+bool TGParser::addDef(std::unique_ptr<Record> Rec, Init *DefmName) {
+  IterSet IterVals;
+
   if (Loops.empty())
-    return false;
+    return addDefOne(std::move(Rec), DefmName, IterVals);
 
-  // We want to instantiate a new copy of CurRec for each combination
-  // of nested loop iterator values.  We don't want top instantiate
-  // any copies until we have values for each loop iterator.
-  IterSet IterVals;
-  return ProcessForeachDefs(CurRec, Loc, IterVals);
+  return addDefForeach(Rec.get(), DefmName, IterVals);
 }
 
-/// ProcessForeachDefs - Given a record, a loop and a loop iterator,
-/// apply each of the variable values in this loop and then process
-/// subloops.
-bool TGParser::ProcessForeachDefs(Record *CurRec, SMLoc Loc, IterSet &IterVals){
-  // Recursively build a tuple of iterator values.
+/// Recursive helper function for addDef/addDefOne to resolve references to
+/// foreach variables.
+bool TGParser::addDefForeach(Record *Rec, Init *DefmName, IterSet &IterVals) {
   if (IterVals.size() != Loops.size()) {
     assert(IterVals.size() < Loops.size());
     ForeachLoop &CurLoop = Loops[IterVals.size()];
-    ListInit *List = dyn_cast<ListInit>(CurLoop.ListValue);
-    if (!List) {
-      Error(Loc, "Loop list is not a list");
-      return true;
-    }
+    ListInit *List = CurLoop.ListValue;
 
     // Process each value.
     for (unsigned i = 0; i < List->size(); ++i) {
-      RecordResolver R(*CurRec);
-      Init *ItemVal = List->getElement(i)->resolveReferences(R);
-      IterVals.push_back(IterRecord(CurLoop.IterVar, ItemVal));
-      if (ProcessForeachDefs(CurRec, Loc, IterVals))
+      IterVals.push_back(IterRecord(CurLoop.IterVar, List->getElement(i)));
+      if (addDefForeach(Rec, DefmName, IterVals))
         return true;
       IterVals.pop_back();
     }
     return false;
   }
 
   // This is the bottom of the recursion. We have all of the iterator values
   // for this point in the iteration space.  Instantiate a new record to
   // reflect this combination of values.
-  auto IterRec = make_unique<Record>(*CurRec);
-
-  // Set the iterator values now.
-  for (IterRecord &IR : IterVals) {
-    VarInit *IterVar = IR.IterVar;
-    TypedInit *IVal = dyn_cast<TypedInit>(IR.IterValue);
-    if (!IVal)
-      return Error(Loc, "foreach iterator value is untyped");
+  auto IterRec = make_unique<Record>(*Rec);
+  return addDefOne(std::move(IterRec), DefmName, IterVals);
+}
 
-    IterRec->addValue(RecordVal(IterVar->getNameInit(), IVal->getType(), false));
+/// After resolving foreach loops, add the record as a prototype to the
+/// current multiclass, or resolve fully and add to the record keeper.
+bool TGParser::addDefOne(std::unique_ptr<Record> Rec, Init *DefmName,
+                         IterSet &IterVals) {
+  MapResolver R(Rec.get());
 
-    if (SetValue(IterRec.get(), Loc, IterVar->getNameInit(), None, IVal))
-      return Error(Loc, "when instantiating this def");
+  for (IterRecord &IR : IterVals)
+    R.set(IR.IterVar->getNameInit(), IR.IterValue);
 
-    // Resolve it next.
-    IterRec->resolveReferencesTo(IterRec->getValue(IterVar->getNameInit()));
+  Rec->resolveReferences(R);
 
-    // Remove it.
-    IterRec->removeValue(IterVar->getNameInit());
+  if (CurMultiClass) {
+    for (const auto &Proto : CurMultiClass->DefPrototypes) {
+      if (Proto->getNameInit() == Rec->getNameInit()) {
+        if (!Rec->isAnonymous()) {
+          PrintError(Rec->getLoc(),
+                    Twine("def '") + Rec->getNameInitAsString() +
+                        "' already defined in this multiclass!");
+          PrintNote(Proto->getLoc(), "location of previous definition");
+          return true;
+        }
+        Rec->setName(Records.getNewAnonymousName());
+        break;
+      }
+    }
+    CurMultiClass->DefPrototypes.emplace_back(std::move(Rec));
+    return false;
   }
 
-  if (Records.getDef(IterRec->getNameInitAsString())) {
-    // If this record is anonymous, it's no problem, just generate a new name
-    if (!IterRec->isAnonymous())
-      return Error(Loc, "def already exists: " +IterRec->getNameInitAsString());
+  // Name construction is an incoherent mess. Unfortunately, existing .td
+  // files rely on pretty much all the quirks and implementation details of
+  // this.
+  if (DefmName) {
+    MapResolver R(Rec.get());
+    R.set(StringInit::get("NAME"), DefmName);
+    Rec->resolveReferences(R);
+  }
 
-    IterRec->setName(Records.getNewAnonymousName());
+  if (Record *Prev = Records.getDef(Rec->getNameInitAsString())) {
+    if (!Rec->isAnonymous()) {
+      PrintError(Rec->getLoc(),
+                 "def already exists: " + Rec->getNameInitAsString());
+      PrintNote(Prev->getLoc(), "location of previous definition");
+      return true;
     }
+    Rec->setName(Records.getNewAnonymousName());
+  }
+
+  Rec->resolveReferences();
+  checkConcrete(*Rec);
 
-  Record *IterRecSave = IterRec.get(); // Keep a copy before release.
-  Records.addDef(std::move(IterRec));
-  IterRecSave->resolveReferences();
-  checkConcrete(*IterRecSave);
-  if (addToDefsets(*IterRecSave))
+  // If ObjectBody has template arguments, it's an error.
+  assert(Rec->getTemplateArgs().empty() && "How'd this get template args?");
+
+  for (DefsetRecord *Defset : Defsets) {
+    DefInit *I = Rec->getDefInit();
+    if (!I->getType()->typeIsA(Defset->EltTy)) {
+      PrintError(Rec->getLoc(), Twine("adding record of incompatible type '") +
+                                    I->getType()->getAsString() +
+                                     "' to defset");
+      PrintNote(Defset->Loc, "location of defset declaration");
       return true;
+    }
+    Defset->Elements.push_back(I);
+  }
+
+  Records.addDef(std::move(Rec));
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Parser Code
 //===----------------------------------------------------------------------===//
 
 /// isObjectStart - Return true if this is a valid first token for an Object.
 static bool isObjectStart(tgtok::TokKind K) {
   return K == tgtok::Class || K == tgtok::Def || K == tgtok::Defm ||
          K == tgtok::Let || K == tgtok::MultiClass || K == tgtok::Foreach ||
          K == tgtok::Defset;
 }
 
-/// ParseObjectName - If an object name is specified, return it.  Otherwise,
-/// return 0.
+/// ParseObjectName - If a valid object name is specified, return it. If no
+/// name is specified, return the unset initializer. Return nullptr on parse
+/// error.
 ///   ObjectName ::= Value [ '#' Value ]*
 ///   ObjectName ::= /*empty*/
 ///
 Init *TGParser::ParseObjectName(MultiClass *CurMultiClass) {
   switch (Lex.getCode()) {
   case tgtok::colon:
   case tgtok::semi:
   case tgtok::l_brace:
     // These are all of the tokens that can begin an object body.
     // Some of these can also begin values but we disallow those cases
     // because they are unlikely to be useful.
-    return nullptr;
+    return UnsetInit::get();
   default:
     break;
   }
 
   Record *CurRec = nullptr;
   if (CurMultiClass)
     CurRec = &CurMultiClass->Rec;
 
-  RecTy *Type = nullptr;
-  if (CurRec) {
-    const TypedInit *CurRecName = dyn_cast<TypedInit>(CurRec->getNameInit());
-    if (!CurRecName) {
-      TokError("Record name is not typed!");
-      return nullptr;
-    }
-    Type = CurRecName->getType();
-  }
-
-  return ParseValue(CurRec, Type, ParseNameMode);
+  return ParseValue(CurRec, StringRecTy::get(), ParseNameMode);
 }
 
 /// ParseClassID - Parse and resolve a reference to a class name.  This returns
 /// null on error.
 ///
 ///    ClassID ::= ID
 ///
 Record *TGParser::ParseClassID() {
Init *TGParser::ParseIDValue(Record *CurRec, StringInit *Name, SMLoc NameLoc,
   }
 
   if (Mode == ParseNameMode)
     return Name;
 
   if (Init *I = Records.getGlobal(Name->getValue()))
     return I;
 
+  // Allow self-references of concrete defs, but delay the lookup so that we
+  // get the correct type.
+  if (CurRec && !CurMultiClass && CurRec->getNameInit() == Name)
+    return UnOpInit::get(UnOpInit::CAST, Name, CurRec->getType());
+
   if (Mode == ParseValueMode) {
     Error(NameLoc, "Variable not defined: '" + Name->getValue() + "'");
     return nullptr;
   }
 
   return Name;
 }
 
VarInit *TGParser::ParseForeachDeclaration(ListInit *&ForeachListValue) {
   default: {
     SMLoc ValueLoc = Lex.getLoc();
     Init *I = ParseValue(nullptr);
     if (!isa<ListInit>(I)) {
       std::string Type;
       if (TypedInit *TI = dyn_cast<TypedInit>(I))
         Type = (Twine("' of type '") + TI->getType()->getAsString()).str();
       Error(ValueLoc, "expected a list, got '" + I->getAsString() + Type + "'");
+      if (CurMultiClass)
+        PrintNote({}, "references to multiclass template arguments cannot be "
+                      "resolved at this time");
       return nullptr;
     }
     ForeachListValue = dyn_cast<ListInit>(I);
     IterType = ForeachListValue->getElementType();
     break;
   }
   }
 

 ///   DefInst ::= DEF ObjectName ObjectBody
 ///
 bool TGParser::ParseDef(MultiClass *CurMultiClass) {
   SMLoc DefLoc = Lex.getLoc();
   assert(Lex.getCode() == tgtok::Def && "Unknown tok");
   Lex.Lex();  // Eat the 'def' token.
 
   // Parse ObjectName and make a record for it.
-  std::unique_ptr<Record> CurRecOwner;
+  std::unique_ptr<Record> CurRec;
   Init *Name = ParseObjectName(CurMultiClass);
-  if (Name)
-    CurRecOwner = make_unique<Record>(Name, DefLoc, Records);
-  else
-    CurRecOwner = make_unique<Record>(Records.getNewAnonymousName(), DefLoc,
-                                      Records, /*IsAnonymous=*/true);
-  Record *CurRec = CurRecOwner.get(); // Keep a copy since we may release.
-
-  if (!CurMultiClass && Loops.empty()) {
-    // Top-level def definition.
-
-    // Ensure redefinition doesn't happen.
-    if (Records.getDef(CurRec->getNameInitAsString()))
-      return Error(DefLoc, "def '" + CurRec->getNameInitAsString()+
-                   "' already defined");
-    Records.addDef(std::move(CurRecOwner));
-
-    if (ParseObjectBody(CurRec))
-      return true;
-  } else if (CurMultiClass) {
-    // Parse the body before adding this prototype to the DefPrototypes vector.
-    // That way implicit definitions will be added to the DefPrototypes vector
-    // before this object, instantiated prior to defs derived from this object,
-    // and this available for indirect name resolution when defs derived from
-    // this object are instantiated.
-    if (ParseObjectBody(CurRec))
-      return true;
-
-    // Otherwise, a def inside a multiclass, add it to the multiclass.
-    for (const auto &Proto : CurMultiClass->DefPrototypes)
-      if (Proto->getNameInit() == CurRec->getNameInit())
-        return Error(DefLoc, "def '" + CurRec->getNameInitAsString() +
-                     "' already defined in this multiclass!");
-    CurMultiClass->DefPrototypes.push_back(std::move(CurRecOwner));
-  } else if (ParseObjectBody(CurRec)) {
-    return true;
-  }
-
-  if (!CurMultiClass) { // Def's in multiclasses aren't really defs.
-    // See Record::setName().  This resolve step will see any new name
-    // for the def that might have been created when resolving
-    // inheritance, values and arguments above.
-    CurRec->resolveReferences();
-    if (Loops.empty()) {
-      checkConcrete(*CurRec);
-      if (addToDefsets(*CurRec))
-        return true;
-    }
-  }
-
-  // If ObjectBody has template arguments, it's an error.
-  assert(CurRec->getTemplateArgs().empty() && "How'd this get template args?");
-
-  if (CurMultiClass) {
-    // Copy the template arguments for the multiclass into the def.
-    for (Init *TArg : CurMultiClass->Rec.getTemplateArgs()) {
-      const RecordVal *RV = CurMultiClass->Rec.getValue(TArg);
-      assert(RV && "Template arg doesn't exist?");
-      CurRec->addValue(*RV);
-    }
-  }
+  if (!Name)
 
-  if (ProcessForeachDefs(CurRec, DefLoc))
-    return Error(DefLoc, "Could not process loops for def" +
-                 CurRec->getNameInitAsString());
-
-  return false;
-}
+  if (isa<UnsetInit>(Name))
+    CurRec = make_unique<Record>(Records.getNewAnonymousName(), DefLoc, Records,
+                                 /*Anonymous=*/true);
+  else
+    CurRec = make_unique<Record>(Name, DefLoc, Records);
 
-bool TGParser::addToDefsets(Record &R) {
-  for (DefsetRecord *Defset : Defsets) {
-    DefInit *I = R.getDefInit();
-    if (!I->getType()->typeIsA(Defset->EltTy)) {
-      PrintError(R.getLoc(),
-                 Twine("adding record of incompatible type '") +
-                 I->getType()->getAsString() + "' to defset");
-      PrintNote(Defset->Loc, "to this defset");
-      return true;
-    }
-    Defset->Elements.push_back(I);
-  }
-  return false;
+  if (ParseObjectBody(CurRec.get()))
+
+  return addDef(std::move(CurRec), nullptr);
 }
 
 /// ParseDefset - Parse a defset statement.
 ///
 ///   Defset ::= DEFSET Type Id '=' '{' ObjectList '}'
 ///
 bool TGParser::ParseDefset() {
   assert(Lex.getCode() == tgtok::Defset);
if (Lex.getCode() != tgtok::l_brace) {
     }
     Lex.Lex();  // eat the '}'.
   }
 
   CurMultiClass = nullptr;
   return false;
 }
 
-Record *TGParser::InstantiateMulticlassDef(MultiClass &MC, Record *DefProto,
-                                           Init *&DefmPrefix,
-                                           SMRange DefmPrefixRange,
-                                           ArrayRef<Init *> TArgs,
-                                           ArrayRef<Init *> TemplateVals) {
-  // We need to preserve DefProto so it can be reused for later
-  // instantiations, so create a new Record to inherit from it.
-
-  // Add in the defm name.  If the defm prefix is empty, give each
-  // instantiated def a unique name.  Otherwise, if "#NAME#" exists in the
-  // name, substitute the prefix for #NAME#.  Otherwise, use the defm name
-  // as a prefix.
-
-  bool IsAnonymous = false;
-  if (!DefmPrefix) {
-    DefmPrefix = Records.getNewAnonymousName();
-    IsAnonymous = true;
-  }
-
-  Init *DefName = DefProto->getNameInit();
-  StringInit *DefNameString = dyn_cast<StringInit>(DefName);
-
-  if (DefNameString) {
-    // We have a fully expanded string so there are no operators to
-    // resolve.  We should concatenate the given prefix and name.
-    DefName = BinOpInit::getStrConcat(
-        UnOpInit::get(UnOpInit::CAST, DefmPrefix, StringRecTy::get())
-            ->Fold(DefProto),
-        DefName);
-  }
-
-  // Make a trail of SMLocs from the multiclass instantiations.
-  SmallVector<SMLoc, 4> Locs(1, DefmPrefixRange.Start);
-  Locs.append(DefProto->getLoc().begin(), DefProto->getLoc().end());
-  auto CurRec = make_unique<Record>(DefName, Locs, Records, IsAnonymous);
-
-  SubClassReference Ref;
-  Ref.RefRange = DefmPrefixRange;
-  Ref.Rec = DefProto;
-  AddSubClass(CurRec.get(), Ref);
-
-  // Set the value for NAME. We don't resolve references to it 'til later,
-  // though, so that uses in nested multiclass names don't get
-  // confused.
-  if (SetValue(CurRec.get(), Ref.RefRange.Start, StringInit::get("NAME"), None,
-               DefmPrefix, /*AllowSelfAssignment*/true)) {
-    Error(DefmPrefixRange.Start, "Could not resolve " +
-          CurRec->getNameInitAsString() + ":NAME to '" +
-          DefmPrefix->getAsUnquotedString() + "'");
-    return nullptr;
-  }
-
-  // If the DefNameString didn't resolve, we probably have a reference to
-  // NAME and need to replace it. We need to do at least this much greedily,
-  // otherwise nested multiclasses will end up with incorrect NAME expansions.
-  if (!DefNameString) {
-    RecordVal *DefNameRV = CurRec->getValue("NAME");
-    CurRec->resolveReferencesTo(DefNameRV);
-  }
-
-  if (!CurMultiClass) {
-    // Now that we're at the top level, resolve all NAME references
-    // in the resultant defs that weren't in the def names themselves.
-    RecordVal *DefNameRV = CurRec->getValue("NAME");
-    CurRec->resolveReferencesTo(DefNameRV);
-
-    // Check if the name is a complex pattern.
-    // If so, resolve it.
-    DefName = CurRec->getNameInit();
-    DefNameString = dyn_cast<StringInit>(DefName);
-
-    // OK the pattern is more complex than simply using NAME.
-    // Let's use the heavy weaponery.
-    if (!DefNameString) {
-      ResolveMulticlassDefArgs(MC, CurRec.get(), DefmPrefixRange.Start,
-                               Lex.getLoc(), TArgs, TemplateVals,
-                               false/*Delete args*/);
-      DefName = CurRec->getNameInit();
-      DefNameString = dyn_cast<StringInit>(DefName);
-
-      if (!DefNameString) {
-        DefName = DefName->convertInitializerTo(StringRecTy::get());
-        DefNameString = dyn_cast<StringInit>(DefName);
-      }
-
-      if (!DefNameString) {
-        PrintFatalError(CurRec->getLoc()[CurRec->getLoc().size() - 1],
-                        DefName->getAsUnquotedString() + " is not a string.");
-        return nullptr;
-      }
-
-      CurRec->setName(DefName);
-    }
-
-    // Now that NAME references are resolved and we're at the top level of
-    // any multiclass expansions, add the record to the RecordKeeper. If we are
-    // currently in a multiclass, it means this defm appears inside a
-    // multiclass and its name won't be fully resolvable until we see
-    // the top-level defm. Therefore, we don't add this to the
-    // RecordKeeper at this point. If we did we could get duplicate
-    // defs as more than one probably refers to NAME or some other
-    // common internal placeholder.
-
-    // Ensure redefinition doesn't happen.
-    if (Records.getDef(CurRec->getNameInitAsString())) {
-      Error(DefmPrefixRange.Start, "def '" + CurRec->getNameInitAsString() +
-            "' already defined, instantiating defm with subdef '" +
-            DefProto->getNameInitAsString() + "'");
-      return nullptr;
-    }
-
-    Record *CurRecSave = CurRec.get(); // Keep a copy before we release.
-    Records.addDef(std::move(CurRec));
-    return CurRecSave;
-  }
-
-  // FIXME This is bad but the ownership transfer to caller is pretty messy.
-  // The unique_ptr in this function at least protects the exits above.
-  return CurRec.release();
-}
-
-bool TGParser::ResolveMulticlassDefArgs(MultiClass &MC, Record *CurRec,
-                                        SMLoc DefmPrefixLoc, SMLoc SubClassLoc,
-                                        ArrayRef<Init *> TArgs,
-                                        ArrayRef<Init *> TemplateVals,
-                                        bool DeleteArgs) {
-  // Set all template arguments to the specified value or leave them as the
-  // default if necessary, then resolve them all simultaneously.
-  MapResolver R(CurRec);
-
-  for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
-    // Check if a value is specified for this temp-arg.
-    if (i < TemplateVals.size()) {
-      if (SetValue(CurRec, DefmPrefixLoc, TArgs[i], None, TemplateVals[i]))
-        return true;
-    } else if (!CurRec->getValue(TArgs[i])->getValue()->isComplete()) {
-      return Error(SubClassLoc, "value not specified for template argument #" +
-                   Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
-                   ") of multiclassclass '" + MC.Rec.getNameInitAsString() +
-                   "'");
-    }
-
-    R.set(TArgs[i], CurRec->getValue(TArgs[i])->getValue());
-
-    if (DeleteArgs)
-      CurRec->removeValue(TArgs[i]);
-  }
-
-  CurRec->resolveReferences(R);
-
-  return false;
-}
-
-bool TGParser::ResolveMulticlassDef(MultiClass &MC,
-                                    Record *CurRec,
-                                    Record *DefProto,
-                                    SMLoc DefmPrefixLoc) {
-  // If the mdef is inside a 'let' expression, add to each def.
-  if (ApplyLetStack(CurRec))
-    return Error(DefmPrefixLoc, "when instantiating this defm");
-
-  // Don't create a top level definition for defm inside multiclasses,
-  // instead, only update the prototypes and bind the template args
-  // with the new created definition.
-  if (!CurMultiClass)
-    return false;
-  for (const auto &Proto : CurMultiClass->DefPrototypes)
-    if (Proto->getNameInit() == CurRec->getNameInit())
-      return Error(DefmPrefixLoc, "defm '" + CurRec->getNameInitAsString() +
-                   "' already defined in this multiclass!");
-  CurMultiClass->DefPrototypes.push_back(std::unique_ptr<Record>(CurRec));
-
-  // Copy the template arguments for the multiclass into the new def.
-  for (Init * TA : CurMultiClass->Rec.getTemplateArgs()) {
-    const RecordVal *RV = CurMultiClass->Rec.getValue(TA);
-    assert(RV && "Template arg doesn't exist?");
-    CurRec->addValue(*RV);
-  }
-
-  return false;
-}
-
 /// ParseDefm - Parse the instantiation of a multiclass.
 ///
 ///   DefMInst ::= DEFM ID ':' DefmSubClassRef ';'
 ///
 bool TGParser::ParseDefm(MultiClass *CurMultiClass) {
   assert(Lex.getCode() == tgtok::Defm && "Unexpected token!");
-  SMLoc DefmLoc = Lex.getLoc();
-  Init *DefmPrefix = nullptr;
+  Lex.Lex(); // eat the defm
 
-  if (Lex.Lex() == tgtok::Id) {  // eat the defm.
-    DefmPrefix = ParseObjectName(CurMultiClass);
-  }
+  Init *DefmName = ParseObjectName(CurMultiClass);
+  if (!DefmName)
+    return true;
+  if (isa<UnsetInit>(DefmName))
+    DefmName = Records.getNewAnonymousName();
 
-  SMLoc DefmPrefixEndLoc = Lex.getLoc();
   if (Lex.getCode() != tgtok::colon)
     return TokError("expected ':' after defm identifier");
 
   // Keep track of the new generated record definitions.
-  std::vector<Record*> NewRecDefs;
+  SmallVector<std::unique_ptr<Record>, 8> NewRecDefs;
 
   // This record also inherits from a regular class (non-multiclass)?
   bool InheritFromClass = false;
 
   // eat the colon.
   Lex.Lex();
 
   SMLoc SubClassLoc = Lex.getLoc();
while (true) {
     ArrayRef<Init*> TemplateVals = Ref.TemplateArgs;
 
     // Verify that the correct number of template arguments were specified.
     ArrayRef<Init *> TArgs = MC->Rec.getTemplateArgs();
     if (TArgs.size() < TemplateVals.size())
       return Error(SubClassLoc,
                    "more template args specified than multiclass expects");
 
+    DenseMap<Init *, Init *> TemplateArgs;
+    for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
+      if (i < TemplateVals.size()) {
+        TemplateArgs.insert({TArgs[i], TemplateVals[i]});
+      } else {
+        Init *Default = MC->Rec.getValue(TArgs[i])->getValue();
+        if (!Default->isComplete()) {
+          return Error(SubClassLoc,
+                       "value not specified for template argument #" +
+                           Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
+                           ") of multiclass '" + MC->Rec.getNameInitAsString() +
+                           "'");
+        }
+        TemplateArgs.insert({TArgs[i], Default});
+      }
+    }
+
     // Loop over all the def's in the multiclass, instantiating each one.
     for (const std::unique_ptr<Record> &DefProto : MC->DefPrototypes) {
-      // The record name construction goes as follow:
-      //  - If the def name is a string, prepend the prefix.
-      //  - If the def name is a more complex pattern, use that pattern.
-      // As a result, the record is instantiated before resolving
-      // arguments, as it would make its name a string.
-      Record *CurRec = InstantiateMulticlassDef(*MC, DefProto.get(), DefmPrefix,
-                                                SMRange(DefmLoc,
-                                                        DefmPrefixEndLoc),
-                                                TArgs, TemplateVals);
-      if (!CurRec)
-        return true;
+      bool ResolveName = true;
+      auto CurRec = make_unique<Record>(*DefProto);
+      CurRec->appendLoc(SubClassLoc);
 
-      // Now that the record is instantiated, we can resolve arguments.
-      if (ResolveMulticlassDefArgs(*MC, CurRec, DefmLoc, SubClassLoc,
-                                   TArgs, TemplateVals, true/*Delete args*/))
-        return Error(SubClassLoc, "could not instantiate def");
-
-      if (ResolveMulticlassDef(*MC, CurRec, DefProto.get(), DefmLoc))
-        return Error(SubClassLoc, "could not instantiate def");
+      if (StringInit *NameString =
+              dyn_cast<StringInit>(CurRec->getNameInit())) {
+        // We have a fully expanded string so there are no operators to
+        // resolve.  We should concatenate the given prefix and name.
+        //
+        // TODO: This MUST happen before template argument resolution. This
+        //       does not make sense and should be changed, but at the time of
+        //       writing, there are existing .td files which rely on this
+        //       implementation detail. It's a bad idea and should be fixed.
+        //       See test/TableGen/name-resolution-consistency.td for some
+        //       examples.
+        CurRec->setName(BinOpInit::getStrConcat(DefmName, NameString));
+        ResolveName = false;
+      }
 
-      NewRecDefs.push_back(CurRec);
+      MapResolver R(CurRec.get());
+
+      if (ResolveName) {
+        // If the proto's name wasn't resolved, we probably have a reference to
+        // NAME and need to replace it.
+        //
+        // TODO: Whether the name is resolved is basically determined by magic.
+        //       Unfortunately, existing .td files depend on it.
+        R.set(StringInit::get("NAME"), DefmName);
       }
 
+      for (const auto &TArg : TemplateArgs)
+        R.set(TArg.first, TArg.second);
+
+      CurRec->resolveReferences(R);
+
+      NewRecDefs.emplace_back(std::move(CurRec));
+    }
 
     if (Lex.getCode() != tgtok::comma) break;
     Lex.Lex(); // eat ','.
 
     if (Lex.getCode() != tgtok::Id)
       return TokError("expected identifier");
 
     SubClassLoc = Lex.getLoc();
if (InheritFromClass) {
     // regular 'def' and inherit all record values.
     SubClassReference SubClass = ParseSubClassReference(nullptr, false);
     while (true) {
       // Check for error.
       if (!SubClass.Rec) return true;
 
       // Get the expanded definition prototypes and teach them about
       // the record values the current class to inherit has
-      for (Record *CurRec : NewRecDefs) {
+      for (const auto &CurRec : NewRecDefs) {
         // Add it.
-        if (AddSubClass(CurRec, SubClass))
-          return true;
-
-        if (ApplyLetStack(CurRec))
+        if (AddSubClass(CurRec.get(), SubClass))
           return true;
       }
 
       if (Lex.getCode() != tgtok::comma) break;
       Lex.Lex(); // eat ','.
       SubClass = ParseSubClassReference(nullptr, false);
     }
   }
 
-  if (!CurMultiClass) {
-    for (Record *CurRec : NewRecDefs) {
-      // See Record::setName().  This resolve step will see any new
-      // name for the def that might have been created when resolving
-      // inheritance, values and arguments above.
-      CurRec->resolveReferences();
-      checkConcrete(*CurRec);
-      if (addToDefsets(*CurRec))
-        return true;
-    }
+  for (auto &CurRec : NewRecDefs) {
+    if (ApplyLetStack(CurRec.get()))
+
+    addDef(std::move(CurRec), DefmName);
   }
 
   if (Lex.getCode() != tgtok::semi)
     return TokError("expected ';' at end of defm");
   Lex.Lex();
 
   return false;
 }

llvm/trunk/test/TableGen/MultiClass-defm-fail.td

 multiclass M1<string s> {
   def _r1 : A;
   // It would be nice if we could refer to _r1's name without having to pass it
   // explicitly via 's'.
   // XCHECK-DAG: note: instantiated from multiclass
   defm _m1: M0<s # "_r1">;
 }
 
-// CHECK: defm d1: M1
+// CHECK: def _m01: B
 // CHECK: note: instantiated from multiclass
 // CHECK: defm _m1: M0
 // CHECK: note: instantiated from multiclass
-// CHECK: def _m01: B
+// CHECK: defm d1: M1
 defm d1: M1<"d1">;

llvm/trunk/test/TableGen/foreach-multiclass.td

+// RUN: llvm-tblgen %s | FileCheck %s
+// XFAIL: vg_leak
+
+// CHECK: --- Defs ---
+
+// CHECK: def A00 {
+// CHECK:   int sum = 7;
+// CHECK: }
+
+// CHECK: def A01 {
+// CHECK:   int sum = 8;
+// CHECK: }
+
+multiclass A<int x> {
+  // Allow foreach in multiclass as long as the list does not depend on
+  // template args.
+  foreach i = [0, 1] in {
+    def NAME#i {
+      int sum = !add(x, i);
+    }
+  }
+}
+
+defm A0 : A<7>;

llvm/trunk/test/TableGen/name-resolution-consistency.td

+// RUN: llvm-tblgen %s | FileCheck %s
+// XFAIL: vg_leak
+
+// This test demonstrates a number of inconsistencies in how NAME is resolved
+// and record names are constructed.
+//
+// The TODO lines describe a suggested consistent behavior that would result
+// from:
+//  (1) Treating NAME as an implicit multiclass template argument and
+//  (2) always storing the name of (non-anonymous) prototype records in
+//      multiclasses with at least one explicit reference to NAME.
+//
+// Unfortunately, several backends (including X86) rely quite heavily on the
+// current inconsistent behavior and would have to be fixed.
+
+// CHECK: def B0a {
+// CHECK:   string e = "B0";
+// CHECK: }
+
+// CHECK: def B0ba {
+// TODO: expect "B0b" here
+// CHECK:   string a = "B0";
+// CHECK:   string b = "B0";
+// CHECK: }
+
+// CHECK: def B0cza {
+// TODO: expect "B0cz" here
+// CHECK:   string a = "B0";
+// CHECK:   string b = "B0";
+// CHECK: }
+
+// TODO: expect this to be named 'xB0b'
+// CHECK: def B0xb {
+// TODO: expect "B0b" here
+// CHECK:   string c = "b";
+// CHECK:   string d = "b";
+// CHECK: }
+
+// TODO: expect this to be named B0bys
+// CHECK: def B0ys {
+// TODO: expect "B0b" here
+// CHECK:   string f = "b";
+// CHECK:   string g = "b";
+// CHECK: }
+
+// CHECK: def xB0cz {
+// CHECK:   string c = "B0cz";
+// CHECK:   string d = "B0cz";
+// CHECK: }
+
+// TODO: expect this to be named B0czyt
+// CHECK: def yt {
+// CHECK:   string f = "B0cz";
+// CHECK:   string g = "B0cz";
+// CHECK: }
+
+multiclass A<string p, string q> {
+  def a {
+    string a = NAME;
+    string b = p;
+  }
+
+  def x # NAME {
+    string c = NAME;
+    string d = p;
+  }
+
+  def y # q {
+    string f = NAME;
+    string g = p;
+  }
+}
+
+multiclass B<string name, string t> {
+  def a {
+    string e = NAME;
+  }
+
+  defm b : A<NAME, "s">;
+
+  defm NAME # c # name : A<NAME, t>;
+}
+
+defm B0 : B<"z", "t">;

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

 // CHECK: def D0 {
 // CHECK:   D d = D0;
 // CHECK: }
 
 // CHECK: def E0 {
 // CHECK:   E e = E0;
 // CHECK: }
 
+// CHECK: def F0 {
+// CHECK:   Fa as_a = F0;
+// CHECK:   Fb as_b = F0;
+// CHECK: }
+// CHECK: def F0x {
+// CHECK:   Fc as_c = F0;
+// CHECK: }
+
 def ops;
 
 class A<dag d> {
   dag a = d;
 }
 
 // This type of self-reference is used in various places defining register
 // classes.

 
 // Putting the !cast directly in the def should work as well: we shouldn't
 // depend on implementation details of when exactly the record is looked up.
 //
 // Note the difference between !cast<E>("E0") and plain E0: the latter wouldn't
 // work here because E0 does not yet have E as a superclass while the template
 // arguments are being parsed.
 def E0 : E<!cast<E>("E0")>;
+
+// Ensure that records end up with the correct type even when direct self-
+// references are involved.
+class Fa;
+class Fb<Fa x> {
+  Fa as_a = x;
+}
+class Fc<Fb x> {
+  Fb as_b = x;
+}
+
+def F0 : Fa, Fb<F0>, Fc<F0>;
+def F0x {
+  Fc as_c = F0;
+}