TableGen: Streamline how defs are instantiated

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

Reviewers: arsenm, craig.topper, tra, MartinO

Subscribers: wdng, llvm-commits

Differential Revision: https://reviews.llvm.org/D44478

llvm-svn: 328117

llvm/include/llvm/TableGen/Record.h

@@ -1370,9 +1370,8 @@ public:
     init();
   }
 
-  explicit Record(StringRef N, ArrayRef<SMLoc> locs, RecordKeeper &records,
+  explicit Record(StringRef N, ArrayRef<SMLoc> locs, RecordKeeper &records)
-                  bool Anonymous = false)
+      : Record(StringInit::get(N), locs, records) {}
-    : Record(StringInit::get(N), locs, records, Anonymous) {}
 
   // 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
@@ -1400,6 +1399,10 @@ public:
   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();
@@ -1491,6 +1494,7 @@ public:
   }
 
   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));
   }

llvm/lib/TableGen/Record.cpp

@@ -1508,14 +1508,8 @@ Init *VarListElementInit::getBit(unsigned Bit) const {
   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();
@@ -1865,7 +1859,14 @@ 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() {

llvm/lib/TableGen/TGParser.cpp

@@ -337,40 +337,32 @@ bool TGParser::AddSubMultiClass(MultiClass *CurMC,
   return false;
 }
 
-/// ProcessForeachDefs - Given a record, apply all of the variable
+/// Add a record that results from 'def' or 'defm', after template arguments
-/// values in all surrounding foreach loops, creating new records for
+/// and the external let stack have been resolved.
-/// each combination of values.
+///
-bool TGParser::ProcessForeachDefs(Record *CurRec, SMLoc Loc) {
+/// Apply foreach loops, resolve internal variable references, and add to the
-  if (Loops.empty())
+/// current multi class or the global record keeper as appropriate.
-    return false;
+bool TGParser::addDef(std::unique_ptr<Record> Rec, Init *DefmName) {
-
-  // 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);
+
+  if (Loops.empty())
+    return addDefOne(std::move(Rec), DefmName, IterVals);
+
+  return addDefForeach(Rec.get(), DefmName, IterVals);
 }
 
-/// ProcessForeachDefs - Given a record, a loop and a loop iterator,
+/// Recursive helper function for addDef/addDefOne to resolve references to
-/// apply each of the variable values in this loop and then process
+/// foreach variables.
-/// subloops.
+bool TGParser::addDefForeach(Record *Rec, Init *DefmName, IterSet &IterVals) {
-bool TGParser::ProcessForeachDefs(Record *CurRec, SMLoc Loc, IterSet &IterVals){
-  // Recursively build a tuple of iterator values.
   if (IterVals.size() != Loops.size()) {
     assert(IterVals.size() < Loops.size());
     ForeachLoop &CurLoop = Loops[IterVals.size()];
-    ListInit *List = dyn_cast<ListInit>(CurLoop.ListValue);
+    ListInit *List = CurLoop.ListValue;
-    if (!List) {
-      Error(Loc, "Loop list is not a list");
-      return true;
-    }
 
     // Process each value.
     for (unsigned i = 0; i < List->size(); ++i) {
-      RecordResolver R(*CurRec);
+      IterVals.push_back(IterRecord(CurLoop.IterVar, List->getElement(i)));
-      Init *ItemVal = List->getElement(i)->resolveReferences(R);
+      if (addDefForeach(Rec, DefmName, IterVals))
-      IterVals.push_back(IterRecord(CurLoop.IterVar, ItemVal));
-      if (ProcessForeachDefs(CurRec, Loc, IterVals))
         return true;
       IterVals.pop_back();
     }
@@ -380,41 +372,77 @@ bool TGParser::ProcessForeachDefs(Record *CurRec, SMLoc Loc, IterSet &IterVals){
   // 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);
+  auto IterRec = make_unique<Record>(*Rec);
+  return addDefOne(std::move(IterRec), DefmName, IterVals);
+}
 
-  // Set the iterator values now.
+/// After resolving foreach loops, add the record as a prototype to the
-  for (IterRecord &IR : IterVals) {
+/// current multiclass, or resolve fully and add to the record keeper.
-    VarInit *IterVar = IR.IterVar;
+bool TGParser::addDefOne(std::unique_ptr<Record> Rec, Init *DefmName,
-    TypedInit *IVal = dyn_cast<TypedInit>(IR.IterValue);
+                         IterSet &IterVals) {
-    if (!IVal)
+  MapResolver R(Rec.get());
-      return Error(Loc, "foreach iterator value is untyped");
 
-    IterRec->addValue(RecordVal(IterVar->getNameInit(), IVal->getType(), false));
+  for (IterRecord &IR : IterVals)
+    R.set(IR.IterVar->getNameInit(), IR.IterValue);
 
-    if (SetValue(IterRec.get(), Loc, IterVar->getNameInit(), None, IVal))
+  Rec->resolveReferences(R);
-      return Error(Loc, "when instantiating this def");
 
-    // Resolve it next.
+  if (CurMultiClass) {
-    IterRec->resolveReferencesTo(IterRec->getValue(IterVar->getNameInit()));
+    for (const auto &Proto : CurMultiClass->DefPrototypes) {
-
+      if (Proto->getNameInit() == Rec->getNameInit()) {
-    // Remove it.
+        if (!Rec->isAnonymous()) {
-    IterRec->removeValue(IterVar->getNameInit());
+          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())) {
+  // Name construction is an incoherent mess. Unfortunately, existing .td
-    // If this record is anonymous, it's no problem, just generate a new name
+  // files rely on pretty much all the quirks and implementation details of
-    if (!IterRec->isAnonymous())
+  // this.
-      return Error(Loc, "def already exists: " +IterRec->getNameInitAsString());
+  if (DefmName) {
-
+    MapResolver R(Rec.get());
-    IterRec->setName(Records.getNewAnonymousName());
+    R.set(StringInit::get("NAME"), DefmName);
+    Rec->resolveReferences(R);
   }
 
-  Record *IterRecSave = IterRec.get(); // Keep a copy before release.
+  if (Record *Prev = Records.getDef(Rec->getNameInitAsString())) {
-  Records.addDef(std::move(IterRec));
+    if (!Rec->isAnonymous()) {
-  IterRecSave->resolveReferences();
+      PrintError(Rec->getLoc(),
-  checkConcrete(*IterRecSave);
+                 "def already exists: " + Rec->getNameInitAsString());
-  if (addToDefsets(*IterRecSave))
+      PrintNote(Prev->getLoc(), "location of previous definition");
-    return true;
+      return true;
+    }
+    Rec->setName(Records.getNewAnonymousName());
+  }
+
+  Rec->resolveReferences();
+  checkConcrete(*Rec);
+
+  // 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;
 }
 
@@ -429,8 +457,9 @@ static bool isObjectStart(tgtok::TokKind K) {
          K == tgtok::Defset;
 }
 
-/// ParseObjectName - If an object name is specified, return it.  Otherwise,
+/// ParseObjectName - If a valid object name is specified, return it. If no
-/// return 0.
+/// name is specified, return the unset initializer. Return nullptr on parse
+/// error.
 ///   ObjectName ::= Value [ '#' Value ]*
 ///   ObjectName ::= /*empty*/
 ///
@@ -442,7 +471,7 @@ Init *TGParser::ParseObjectName(MultiClass *CurMultiClass) {
     // 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;
   }
@@ -451,17 +480,7 @@ Init *TGParser::ParseObjectName(MultiClass *CurMultiClass) {
   if (CurMultiClass)
     CurRec = &CurMultiClass->Rec;
 
-  RecTy *Type = nullptr;
+  return ParseValue(CurRec, StringRecTy::get(), ParseNameMode);
-  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);
 }
 
 /// ParseClassID - Parse and resolve a reference to a class name.  This returns
@@ -811,6 +830,11 @@ Init *TGParser::ParseIDValue(Record *CurRec, StringInit *Name, SMLoc NameLoc,
   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;
@@ -2224,6 +2248,9 @@ VarInit *TGParser::ParseForeachDeclaration(ListInit *&ForeachListValue) {
       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);
@@ -2416,89 +2443,21 @@ bool TGParser::ParseDef(MultiClass *CurMultiClass) {
   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)
+  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.
+  if (isa<UnsetInit>(Name))
-    // See Record::setName().  This resolve step will see any new name
+    CurRec = make_unique<Record>(Records.getNewAnonymousName(), DefLoc, Records,
-    // for the def that might have been created when resolving
+                                 /*Anonymous=*/true);
-    // inheritance, values and arguments above.
+  else
-    CurRec->resolveReferences();
+    CurRec = make_unique<Record>(Name, DefLoc, Records);
-    if (Loops.empty()) {
-      checkConcrete(*CurRec);
-      if (addToDefsets(*CurRec))
-        return true;
-    }
-  }
 
-  // If ObjectBody has template arguments, it's an error.
+  if (ParseObjectBody(CurRec.get()))
-  assert(CurRec->getTemplateArgs().empty() && "How'd this get template args?");
+    return true;
 
-  if (CurMultiClass) {
+  return addDef(std::move(CurRec), nullptr);
-    // 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 (ProcessForeachDefs(CurRec, DefLoc))
-    return Error(DefLoc, "Could not process loops for def" +
-                 CurRec->getNameInitAsString());
-
-  return false;
-}
-
-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;
 }
 
 /// ParseDefset - Parse a defset statement.
@@ -2818,207 +2777,25 @@ bool TGParser::ParseMultiClass() {
   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();
+  Lex.Lex(); // eat the defm
-  Init *DefmPrefix = nullptr;
 
-  if (Lex.Lex() == tgtok::Id) {  // eat the defm.
+  Init *DefmName = ParseObjectName(CurMultiClass);
-    DefmPrefix = 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;
@@ -3045,31 +2822,62 @@ bool TGParser::ParseDefm(MultiClass *CurMultiClass) {
       return Error(SubClassLoc,
                    "more template args specified than multiclass expects");
 
-    // Loop over all the def's in the multiclass, instantiating each one.
+    DenseMap<Init *, Init *> TemplateArgs;
-    for (const std::unique_ptr<Record> &DefProto : MC->DefPrototypes) {
+    for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
-      // The record name construction goes as follow:
+      if (i < TemplateVals.size()) {
-      //  - If the def name is a string, prepend the prefix.
+        TemplateArgs.insert({TArgs[i], TemplateVals[i]});
-      //  - If the def name is a more complex pattern, use that pattern.
+      } else {
-      // As a result, the record is instantiated before resolving
+        Init *Default = MC->Rec.getValue(TArgs[i])->getValue();
-      // arguments, as it would make its name a string.
+        if (!Default->isComplete()) {
-      Record *CurRec = InstantiateMulticlassDef(*MC, DefProto.get(), DefmPrefix,
+          return Error(SubClassLoc,
-                                                SMRange(DefmLoc,
+                       "value not specified for template argument #" +
-                                                        DefmPrefixEndLoc),
+                           Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
-                                                TArgs, TemplateVals);
+                           ") of multiclass '" + MC->Rec.getNameInitAsString() +
-      if (!CurRec)
+                           "'");
-        return true;
+        }
-
+        TemplateArgs.insert({TArgs[i], Default});
-      // 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");
-
-      NewRecDefs.push_back(CurRec);
     }
 
+    // Loop over all the def's in the multiclass, instantiating each one.
+    for (const std::unique_ptr<Record> &DefProto : MC->DefPrototypes) {
+      bool ResolveName = true;
+      auto CurRec = make_unique<Record>(*DefProto);
+      CurRec->appendLoc(SubClassLoc);
+
+      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;
+      }
+
+      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 ','.
@@ -3099,12 +2907,9 @@ bool TGParser::ParseDefm(MultiClass *CurMultiClass) {
 
       // 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))
+        if (AddSubClass(CurRec.get(), SubClass))
-          return true;
-
-        if (ApplyLetStack(CurRec))
           return true;
       }
 
@@ -3114,16 +2919,11 @@ bool TGParser::ParseDefm(MultiClass *CurMultiClass) {
     }
   }
 
-  if (!CurMultiClass) {
+  for (auto &CurRec : NewRecDefs) {
-    for (Record *CurRec : NewRecDefs) {
+    if (ApplyLetStack(CurRec.get()))
-      // See Record::setName().  This resolve step will see any new
+      return true;
-      // name for the def that might have been created when resolving
+
-      // inheritance, values and arguments above.
+    addDef(std::move(CurRec), DefmName);
-      CurRec->resolveReferences();
-      checkConcrete(*CurRec);
-      if (addToDefsets(*CurRec))
-        return true;
-    }
   }
 
   if (Lex.getCode() != tgtok::semi)

llvm/lib/TableGen/TGParser.h

@@ -126,28 +126,16 @@ private:  // Semantic analysis methods.
   // iteration space.
   typedef std::vector<IterRecord> IterSet;
 
-  bool ProcessForeachDefs(Record *CurRec, SMLoc Loc);
+  bool addDefOne(std::unique_ptr<Record> Rec, Init *DefmName,
-  bool ProcessForeachDefs(Record *CurRec, SMLoc Loc, IterSet &IterVals);
+                 IterSet &IterVals);
-
+  bool addDefForeach(Record *Rec, Init *DefmName, IterSet &IterVals);
-  bool addToDefsets(Record &R);
+  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();

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

@@ -24,9 +24,9 @@ multiclass M1<string s> {
   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/test/TableGen/foreach-multiclass.td

@@ -0,0 +1,24 @@
+// 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/test/TableGen/name-resolution-consistency.td

@@ -0,0 +1,84 @@
+// 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/test/TableGen/self-reference.td

@@ -24,6 +24,14 @@
 // 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> {
@@ -73,3 +81,18 @@ class E<E x> {
 // 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;
+}