[llvm] r328117 - TableGen: Streamline how defs are instantiated

Nicolai Haehnle via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 21 10:12:54 PDT 2018


Author: nha
Date: Wed Mar 21 10:12:53 2018
New Revision: 328117

URL: http://llvm.org/viewvc/llvm-project?rev=328117&view=rev
Log:
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

Added:
    llvm/trunk/test/TableGen/foreach-multiclass.td
    llvm/trunk/test/TableGen/name-resolution-consistency.td
Modified:
    llvm/trunk/include/llvm/TableGen/Record.h
    llvm/trunk/lib/TableGen/Record.cpp
    llvm/trunk/lib/TableGen/TGParser.cpp
    llvm/trunk/lib/TableGen/TGParser.h
    llvm/trunk/test/TableGen/MultiClass-defm-fail.td
    llvm/trunk/test/TableGen/self-reference.td

Modified: llvm/trunk/include/llvm/TableGen/Record.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TableGen/Record.h?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/include/llvm/TableGen/Record.h (original)
+++ llvm/trunk/include/llvm/TableGen/Record.h Wed Mar 21 10:12:53 2018
@@ -1370,9 +1370,8 @@ public:
     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
@@ -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));
   }

Modified: llvm/trunk/lib/TableGen/Record.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/Record.cpp?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/lib/TableGen/Record.cpp (original)
+++ llvm/trunk/lib/TableGen/Record.cpp Wed Mar 21 10:12:53 2018
@@ -1508,14 +1508,8 @@ Init *VarListElementInit::getBit(unsigne
   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() {

Modified: llvm/trunk/lib/TableGen/TGParser.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/lib/TableGen/TGParser.cpp (original)
+++ llvm/trunk/lib/TableGen/TGParser.cpp Wed Mar 21 10:12:53 2018
@@ -337,40 +337,32 @@ bool TGParser::AddSubMultiClass(MultiCla
   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();
     }
@@ -380,41 +372,77 @@ bool TGParser::ProcessForeachDefs(Record
   // 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.
-  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");
+/// 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());
 
-    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))
-      return Error(Loc, "when instantiating this def");
+  Rec->resolveReferences(R);
 
-    // Resolve it next.
-    IterRec->resolveReferencesTo(IterRec->getValue(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;
+  }
+
+  // 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);
+  }
 
-    // Remove it.
-    IterRec->removeValue(IterVar->getNameInit());
+  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());
   }
 
-  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());
+  Rec->resolveReferences();
+  checkConcrete(*Rec);
+
+  // If ObjectBody has template arguments, it's an error.
+  assert(Rec->getTemplateArgs().empty() && "How'd this get template args?");
 
-    IterRec->setName(Records.getNewAnonymousName());
+  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);
   }
 
-  Record *IterRecSave = IterRec.get(); // Keep a copy before release.
-  Records.addDef(std::move(IterRec));
-  IterRecSave->resolveReferences();
-  checkConcrete(*IterRecSave);
-  if (addToDefsets(*IterRecSave))
-    return true;
+  Records.addDef(std::move(Rec));
   return false;
 }
 
@@ -429,8 +457,9 @@ static bool isObjectStart(tgtok::TokKind
          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*/
 ///
@@ -442,7 +471,7 @@ Init *TGParser::ParseObjectName(MultiCla
     // 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(MultiCla
   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
@@ -811,6 +830,11 @@ Init *TGParser::ParseIDValue(Record *Cur
   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::ParseForeachDeclarati
       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 *CurM
   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)) {
+  if (!Name)
     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 (ProcessForeachDefs(CurRec, DefLoc))
-    return Error(DefLoc, "Could not process loops for def" +
-                 CurRec->getNameInitAsString());
+  if (isa<UnsetInit>(Name))
+    CurRec = make_unique<Record>(Records.getNewAnonymousName(), DefLoc, Records,
+                                 /*Anonymous=*/true);
+  else
+    CurRec = make_unique<Record>(Name, DefLoc, Records);
 
-  return false;
-}
+  if (ParseObjectBody(CurRec.get()))
+    return true;
 
-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;
+  return addDef(std::move(CurRec), nullptr);
 }
 
 /// 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();
-  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;
@@ -3045,32 +2822,63 @@ bool TGParser::ParseDefm(MultiClass *Cur
       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);
+
+      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);
+      }
 
-      // 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");
+      for (const auto &TArg : TemplateArgs)
+        R.set(TArg.first, TArg.second);
 
-      if (ResolveMulticlassDef(*MC, CurRec, DefProto.get(), DefmLoc))
-        return Error(SubClassLoc, "could not instantiate def");
+      CurRec->resolveReferences(R);
 
-      NewRecDefs.push_back(CurRec);
+      NewRecDefs.emplace_back(std::move(CurRec));
     }
 
-
     if (Lex.getCode() != tgtok::comma) break;
     Lex.Lex(); // eat ','.
 
@@ -3099,12 +2907,9 @@ bool TGParser::ParseDefm(MultiClass *Cur
 
       // 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;
       }
 
@@ -3114,16 +2919,11 @@ bool TGParser::ParseDefm(MultiClass *Cur
     }
   }
 
-  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()))
+      return true;
+
+    addDef(std::move(CurRec), DefmName);
   }
 
   if (Lex.getCode() != tgtok::semi)

Modified: llvm/trunk/lib/TableGen/TGParser.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.h?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/lib/TableGen/TGParser.h (original)
+++ llvm/trunk/lib/TableGen/TGParser.h Wed Mar 21 10:12:53 2018
@@ -126,28 +126,16 @@ private:  // Semantic analysis methods.
   // 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();

Modified: llvm/trunk/test/TableGen/MultiClass-defm-fail.td
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/MultiClass-defm-fail.td?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/test/TableGen/MultiClass-defm-fail.td (original)
+++ llvm/trunk/test/TableGen/MultiClass-defm-fail.td Wed Mar 21 10:12:53 2018
@@ -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">;

Added: llvm/trunk/test/TableGen/foreach-multiclass.td
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/foreach-multiclass.td?rev=328117&view=auto
==============================================================================
--- llvm/trunk/test/TableGen/foreach-multiclass.td (added)
+++ llvm/trunk/test/TableGen/foreach-multiclass.td Wed Mar 21 10:12:53 2018
@@ -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>;

Added: llvm/trunk/test/TableGen/name-resolution-consistency.td
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/name-resolution-consistency.td?rev=328117&view=auto
==============================================================================
--- llvm/trunk/test/TableGen/name-resolution-consistency.td (added)
+++ llvm/trunk/test/TableGen/name-resolution-consistency.td Wed Mar 21 10:12:53 2018
@@ -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">;

Modified: llvm/trunk/test/TableGen/self-reference.td
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/self-reference.td?rev=328117&r1=328116&r2=328117&view=diff
==============================================================================
--- llvm/trunk/test/TableGen/self-reference.td (original)
+++ llvm/trunk/test/TableGen/self-reference.td Wed Mar 21 10:12:53 2018
@@ -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;
+}




More information about the llvm-commits mailing list