Add the TableGen assert statement, step 3

[Paul-C-Anagnostopoulos]

Summary

This third step adds support for the 'assert' statement in multiclasses. The statement is now supported at top level and in records, classes, and multiclasses.

@jansvoboda11, once this is pushed, you can add your assertions.

[jansvoboda11]

Thanks for working on this, Paul!

I left a couple of questions/comments inline.

Also, it would be nice to go through the clang-format and clang-tidy suggestions and applying those that fit into the TableGen coding style.

llvm/lib/TableGen/TGParser.cpp

368

Replacing using AssertionTuple = std::tuple<SMLoc, Init *, Init *> with a regular struct with named members would make this easier to read/understand. Is there any particular reason you chose std::tuple instead?

428

Nit: superfluous newline?

3331

This patch doesn't seem to change the (supposedly) recursive nature of multiclasses. If the comment is stale, I think it would be clearer to move the change into a separate NFC commit explaining the situation.

3445

It looks like the branches used to be lexicographically ordered. If there's no specific benefit to having this be the last element, it might be better to keep it at the top. WDYT?

llvm/lib/TableGen/TGParser.h

41

Shouldn't this be an union {} or llvm::PointerUnion, since it only holds one type at a time? If so, it would be nice to extract into a prep patch.

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/TGParser.cpp

368

No good reason to use a tuple. The next step is to switch to a struct.

428

I like a line break between two arms of an 'if' when some arms are many lines.

3331

A separate revision just to remove an incorrect comment? Okay.

3445

I will order them alphabetically.

llvm/lib/TableGen/TGParser.h

41

I will investigate this when I convert the assertion tuple to a struct.

[jansvoboda11]

llvm/lib/TableGen/TGParser.cpp

3331

I think a trivial change like this can be committed without a Phabricator review.

[Paul-C-Anagnostopoulos]

I just pushed this revision and I have no idea how. I meant to push the list slice suffix revision, which I did, and somehow this one went along with it.

[dblaikie]

In D99751#2676479, @Paul-C-Anagnostopoulos wrote:

I just pushed this revision and I have no idea how. I meant to push the list slice suffix revision, which I did, and somehow this one went along with it.

If something's been committed accidentally/without approval, please revert it in a timely manner - if your git client is in some difficult state that might take time to untangle, it may be best to make a new/separate checkout just so you can revert that patch sooner rather than later.

[Paul-C-Anagnostopoulos]

The reason I haven't reverted this patch is because I don't feel comfortable doing a revert with the tangled up branches I have. Do you think it's safe to revert the commit on my 'assert' branch even though it is not the topmost commit?

[dblaikie]

In D99751#2677328, @Paul-C-Anagnostopoulos wrote:

The reason I haven't reverted this patch is because I don't feel comfortable doing a revert with the tangled up branches I have. Do you think it's safe to revert the commit on my 'assert' branch even though it is not the topmost commit?

I'm not sure what state your branches are in, so I can't say for sure - hence the suggestion to start a new/separate/clean git checkout and revert it there (you could then throw away that checkout, or you could port the patches from your tangled up checkout instead of fixing it (admittedly fixing it's probably a good experience to figure out what went wrong/how to fix it/etc - but it's not uncommon for folks (myself included) to sometimes just blow things away and start again (porting patches by hand) if it's gotten all messed up))

[Paul-C-Anagnostopoulos]

Ah, so I take it from what you're saying that I can start a new branch and then do a revert of an arbitrary commit on it? I guess there is no reason why not.

[Paul-C-Anagnostopoulos]

Okay, I have reverted this revision.

[dblaikie]

In D99751#2677363, @Paul-C-Anagnostopoulos wrote:

Ah, so I take it from what you're saying that I can start a new branch and then do a revert of an arbitrary commit on it? I guess there is no reason why not.

Actually I meant a whole new git checkout ( https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git ) - but if your current checkout isn't so messed up that you aren't sure a new branch would be enough, then that's great too!

[Paul-C-Anagnostopoulos]

I addressed some of the comments and the relevant clang clean-ups.

[Paul-C-Anagnostopoulos]

Could I get an LGTM on this revision?

[jansvoboda11]

Sorry, I didn't realize that by "next step" you were referring to a future patch. I'd like to see my two comments addressed in this (and a prep) patch, instead of in the future.

llvm/lib/TableGen/TGParser.cpp

368

Great, I think it would be best to do so in this patch.

llvm/lib/TableGen/TGParser.h

41

Can you please do it in a separate patch and rebase this one on top?

[Paul-C-Anagnostopoulos]

Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.

[Paul-C-Anagnostopoulos]

In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.

[jansvoboda11]

In D99751#2688991, @Paul-C-Anagnostopoulos wrote:

In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.

Would introducing a class hierarchy simplify things?

In D99751#2688868, @Paul-C-Anagnostopoulos wrote:

Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.

As a reviewer, I'd be much happier approving following patches:

• patch 1 replaces std::tuple with a well-named members (improvement),
• patch 2 makes RecordsEntry represent exactly one of Record, ForeachLoop, Assertion (improvement),
• patch 3 (this one) enables assertions on multiclasses (improvement).

as opposed to approving this patch that (IMO) exacerbates issues in the existing code, with the promise of future patches that will fix things.

Maybe I'm being too pedantic. Feel free to ping other reviewers, I don't want to block this patch if I'm being unreasonable.

[Paul-C-Anagnostopoulos]

I like your improvements, I just want to do the first one last. It's easier for me to get all the assert-related code in and working, then go back and replace the tuple with a struct when all the relevant code is in one branch. I promise to do it and not just forget about it. It's next on my to-do list.

I'm not sure what you mean by introducing a class hierarchy. A RecordsEntry struct already represents just one of the three items that can appear in it. The question here is whether it's worth making it a union if I have to add a discriminator enum and also, I believe, write a custom destructor for it. Right now the discrimination is made simply by checking which item is non-null.

[jansvoboda11]

LGTM then.

[Paul-C-Anagnostopoulos]

Thanks! I will start a thread on llvm dev about how the union issue.

llvm/docs/TableGen/ProgRef.rst

 
 * In a record definition, the statement is saved and all assertions are
   checked after the record is completely built.
 
 * In a class definition, the assertions are saved and inherited by all
   the subclasses and records that inherit from the class. The assertions are
   then checked when the records are completely built.
 
-* In a multiclass definition, ... [this placement is not yet available]
+* In a multiclass definition, the assertions are saved with the other
+  components of the multiclass and then checked each time the multiclass
+  is instantiated with ``defm``.
 
 Using assertions in TableGen files can simplify record checking in TableGen
 backends. Here is an example of an ``assert`` in two class definitions.
 
 .. code-block:: text
 
   class PersonName<string name> {
     assert !le(!size(name), 32), "person name is too long: " # name;

llvm/include/llvm/TableGen/Record.h

 };
 
 inline raw_ostream &operator<<(raw_ostream &OS, const RecordVal &RV) {
   RV.print(OS << "  ");
   return OS;
 }
 
 class Record {
+public:
+  using AssertionTuple = std::tuple<SMLoc, Init *, Init *>;
+
+private:
   static unsigned LastID;
 
   Init *Name;
   // Location where record was instantiated, followed by the location of
   // multiclass prototypes used.
   SmallVector<SMLoc, 4> Locs;
   SmallVector<Init *, 0> TemplateArgs;
   SmallVector<RecordVal, 0> Values;
   // Vector of [source location, condition Init, message Init].
-  SmallVector<std::tuple<SMLoc, Init *, Init *>, 0> Assertions;
+  SmallVector<AssertionTuple, 0> Assertions;
 
   // All superclasses in the inheritance forest in post-order (yes, it
   // must be a forest; diamond-shaped inheritance is not allowed).
   SmallVector<std::pair<Record *, SMRange>, 0> SuperClasses;
 
   // Tracks Record instances. Not owned by Record.
   RecordKeeper &TrackedRecords;
 
public:
   bool isClass() const { return IsClass; }
 
   ArrayRef<Init *> getTemplateArgs() const {
     return TemplateArgs;
   }
 
   ArrayRef<RecordVal> getValues() const { return Values; }
 
-  ArrayRef<std::tuple<SMLoc, Init *, Init *>> getAssertions() const {
-    return Assertions;
-  }
+  ArrayRef<AssertionTuple> getAssertions() const { return Assertions; }
 
   ArrayRef<std::pair<Record *, SMRange>>  getSuperClasses() const {
     return SuperClasses;
   }
 
   /// Determine whether this record has the specified direct superclass.
   bool hasDirectSuperClass(const Record *SuperClass) const;
 
public:
   void addAssertion(SMLoc Loc, Init *Condition, Init *Message) {
     Assertions.push_back(std::make_tuple(Loc, Condition, Message));
   }
 
   void appendAssertions(const Record *Rec) {
     Assertions.append(Rec->Assertions);
   }
 
-  void checkAssertions();
+  void checkRecordAssertions();
 
   bool isSubClassOf(const Record *R) const {
     for (const auto &SCPair : SuperClasses)
       if (SCPair.first == R)
         return true;
     return false;
   }
 

llvm/lib/TableGen/Record.cpp

NewRec->addSuperClass(Class,
                           SMRange(Class->getLoc().back(),
                                   Class->getLoc().back()));
 
     // Resolve internal references and store in record keeper
     NewRec->resolveReferences();
     Records.addDef(std::move(NewRecOwner));
 
     // Check the assertions.
-    NewRec->checkAssertions();
+    NewRec->checkRecordAssertions();
 
     Def = DefInit::get(NewRec);
   }
 
   return Def;
 }
 
 Init *VarDefInit::resolveReferences(Resolver &R) const {
DagInit *Record::getValueAsDag(StringRef FieldName) const {
   PrintFatalError(getLoc(), "Record `" + getName() + "', field `" +
     FieldName + "' does not have a dag initializer!");
 }
 
 // Check all record assertions: For each one, resolve the condition
 // and message, then call CheckAssert().
 // Note: The condition and message are probably already resolved,
 //       but resolving again allows calls before records are resolved.
-void Record::checkAssertions() {
+void Record::checkRecordAssertions() {
   RecordResolver R(*this);
   R.setFinal(true);
 
   for (auto Assertion : getAssertions()) {
     Init *Condition = std::get<1>(Assertion)->resolveReferences(R);
     Init *Message = std::get<2>(Assertion)->resolveReferences(R);
     CheckAssert(std::get<0>(Assertion), Condition, Message);
   }

llvm/lib/TableGen/TGParser.h

struct LetRecord {
     std::vector<unsigned> Bits;
     Init *Value;
     SMLoc Loc;
     LetRecord(StringInit *N, ArrayRef<unsigned> B, Init *V, SMLoc L)
       : Name(N), Bits(B), Value(V), Loc(L) {
     }
   };
 
-  /// RecordsEntry - Can be either a record or a foreach loop.
+  /// RecordsEntry - Holds exactly one of a Record, ForeachLoop, or
+  /// assertion tuple.
   struct RecordsEntry {
     std::unique_ptr<Record> Rec;
     std::unique_ptr<ForeachLoop> Loop;
+    std::unique_ptr<Record::AssertionTuple> Assertion;
 
     void dump() const;
 
     RecordsEntry() {}
     RecordsEntry(std::unique_ptr<Record> Rec) : Rec(std::move(Rec)) {}
     RecordsEntry(std::unique_ptr<ForeachLoop> Loop)
       : Loop(std::move(Loop)) {}
+    RecordsEntry(std::unique_ptr<Record::AssertionTuple> Assertion)
+      : Assertion(std::move(Assertion)) {}
   };
 
   /// ForeachLoop - Record the iteration state associated with a for loop.
   /// This is used to instantiate items in the loop body.
   ///
   /// IterVar is allowed to be null, in which case no iteration variable is
   /// defined in the loop at all. (This happens when a ForeachLoop is
   /// constructed by desugaring an if statement.)
private:  // Parser methods.
   bool ParseMultiClass();
   bool ParseDefm(MultiClass *CurMultiClass);
   bool ParseDef(MultiClass *CurMultiClass);
   bool ParseDefset();
   bool ParseDefvar();
   bool ParseForeach(MultiClass *CurMultiClass);
   bool ParseIf(MultiClass *CurMultiClass);
   bool ParseIfBody(MultiClass *CurMultiClass, StringRef Kind);
-  bool ParseAssert(MultiClass *CurMultiClass, Record *CurRec);
+  bool ParseAssert(MultiClass *CurMultiClass, Record *CurRec = nullptr);
   bool ParseTopLevelLet(MultiClass *CurMultiClass);
   void ParseLetList(SmallVectorImpl<LetRecord> &Result);
 
   bool ParseObjectBody(Record *CurRec);
   bool ParseBody(Record *CurRec);
   bool ParseBodyItem(Record *CurRec);
 
   bool ParseTemplateArgList(Record *CurRec);

llvm/lib/TableGen/TGParser.cpp

bool TGParser::AddSubMultiClass(MultiClass *CurMC,
   TemplateArgs.emplace_back(
       QualifiedNameOfImplicitName(SMC),
       VarInit::get(QualifiedNameOfImplicitName(CurMC), StringRecTy::get()));
 
   // Add all of the defs in the subclass into the current multiclass.
   return resolve(SMC->Entries, TemplateArgs, false, &CurMC->Entries);
 }
 
-/// Add a record or foreach loop to the current context (global record keeper,
-/// current inner-most foreach loop, or multiclass).
+/// Add a record, foreach loop, or assertion to the current context.
 bool TGParser::addEntry(RecordsEntry E) {
-  assert(!E.Rec || !E.Loop);
+  assert((!!E.Rec + !!E.Loop + !!E.Assertion) == 1 &&
+         "RecordsEntry has invalid number of items");
 
+  // If we are parsing a loop, add it to the loop's entries.
   if (!Loops.empty()) {
     Loops.back()->Entries.push_back(std::move(E));
     return false;
   }
 
+  // If it is a loop, then resolve and perform the loop.
   if (E.Loop) {
     SubstStack Stack;
     return resolve(*E.Loop, Stack, CurMultiClass == nullptr,
                    CurMultiClass ? &CurMultiClass->Entries : nullptr);
   }
 
+  // If we are parsing a multiclass, add it to the multiclass's entries.
   if (CurMultiClass) {
     CurMultiClass->Entries.push_back(std::move(E));
     return false;
   }
 
+  // If it is an assertion, then it's a top-level one, so check it.
+  if (E.Assertion) {
+    CheckAssert(std::get<0>(*E.Assertion), std::get<1>(*E.Assertion), 
+                std::get<2>(*E.Assertion));
+    return false;
+  }
+
+  // It must be a record, so finish it off.
   return addDefOne(std::move(E.Rec));
 }
 
 /// Resolve the entries in \p Loop, going over inner loops recursively
 /// and making the given subsitutions of (name, value) pairs.
 ///
 /// The resulting records are stored in \p Dest if non-null. Otherwise, they
 /// are added to the global record keeper.

 /// are added to the global record keeper.
 bool TGParser::resolve(const std::vector<RecordsEntry> &Source,
                        SubstStack &Substs, bool Final,
                        std::vector<RecordsEntry> *Dest, SMLoc *Loc) {
   bool Error = false;
   for (auto &E : Source) {
     if (E.Loop) {
       Error = resolve(*E.Loop, Substs, Final, Dest);
+
+    } else if (E.Assertion) {
+      MapResolver R;
+      for (const auto &S : Substs)
+        R.set(S.first, S.second);
+      Init *Condition = std::get<1>(*E.Assertion)->resolveReferences(R);
+      Init *Message = std::get<2>(*E.Assertion)->resolveReferences(R);
+
+      if (Dest) {
+        std::unique_ptr<Record::AssertionTuple> Tuple =
+            std::make_unique<Record::AssertionTuple>(std::get<0>(*E.Assertion),
+                                                     std::move(Condition),
+                                                     std::move(Message));
+        Dest->push_back(std::move(Tuple));
+      } else {
+        CheckAssert(std::get<0>(*E.Assertion), Condition, Message);
+      }
+
     } else {
       auto Rec = std::make_unique<Record>(*E.Rec);
       if (Loc)
         Rec->appendLoc(*Loc);
 
       MapResolver R(Rec.get());
       for (const auto &S : Substs)
         R.set(S.first, S.second);
bool TGParser::addDefOne(std::unique_ptr<Record> Rec) {
   if (!isa<StringInit>(Rec->getNameInit())) {
     PrintError(Rec->getLoc(), Twine("record name '") +
                                   Rec->getNameInit()->getAsString() +
                                   "' could not be fully resolved");
     return true;
   }
 
   // Check the assertions.
-  Rec->checkAssertions();
+  Rec->checkRecordAssertions();
 
   // 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 '") +

 bool TGParser::ApplyLetStack(Record *CurRec) {
   for (SmallVectorImpl<LetRecord> &LetInfo : LetStack)
     for (LetRecord &LR : LetInfo)
       if (SetValue(CurRec, LR.Loc, LR.Name, LR.Bits, LR.Value))
         return true;
   return false;
 }
 
+/// Apply the current let bindings to the RecordsEntry.
 bool TGParser::ApplyLetStack(RecordsEntry &Entry) {
   if (Entry.Rec)
     return ApplyLetStack(Entry.Rec.get());
 
+  // Let bindings are not applied to assertions.
+  if (Entry.Assertion)
+    return false;
+
   for (auto &E : Entry.Loop->Entries) {
     if (ApplyLetStack(E))
       return true;
   }
 
   return false;
 }
 
bool TGParser::ParseObjectBody(Record *CurRec) {
   }
 
   if (ApplyLetStack(CurRec))
     return true;
 
   return ParseBody(CurRec);
 }
 
-/// ParseDef - Parse and return a top level or multiclass def, return the record
-/// corresponding to it.  This returns null on error.
+/// ParseDef - Parse and return a top level or multiclass record definition.
+/// Return false if okay, true if error.
 ///
 ///   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.
 
bool TGParser::ParseAssert(MultiClass *CurMultiClass, Record *CurRec) {
 
   Init *Message = ParseValue(CurRec);
   if (!Message)
     return true;
 
   if (!consume(tgtok::semi))
     return TokError("expected ';'");
 
-  if (CurMultiClass) {
-    assert(false && "assert in multiclass not yet supported");
-  } else if (CurRec) {
+  if (CurRec) {
     CurRec->addAssertion(ConditionLoc, Condition, Message);
-  } else { // at top level
-    CheckAssert(ConditionLoc, Condition, Message);
+  } else {
+    std::unique_ptr<Record::AssertionTuple> Tuple =
+         std::make_unique<Record::AssertionTuple>(ConditionLoc, Condition, Message);
+    addEntry(std::move(Tuple));
   }
  
   return false;
 }
 
 /// ParseClass - Parse a tblgen class definition.
 ///
 ///   ClassInst ::= CLASS ID TemplateArgList? ObjectBody
bool TGParser::ParseTopLevelLet(MultiClass *CurMultiClass) {
   return false;
 }
 
 /// ParseMultiClass - Parse a multiclass definition.
 ///
 ///  MultiClassInst ::= MULTICLASS ID TemplateArgList?
 ///                     ':' BaseMultiClassList '{' MultiClassObject+ '}'
 ///  MultiClassObject ::= DefInst
 ///  MultiClassObject ::= MultiClassInst
 ///  MultiClassObject ::= DefMInst
+///  MultiClassObject ::= Defvar
+///  MultiClassObject ::= Foreach
+///  MultiClassObject ::= If
 ///  MultiClassObject ::= LETCommand '{' ObjectList '}'
 ///  MultiClassObject ::= LETCommand Object
+///  MultiClassObject ::= Assert
 ///
 bool TGParser::ParseMultiClass() {
   assert(Lex.getCode() == tgtok::MultiClass && "Unexpected token");
   Lex.Lex();  // Eat the multiclass token.
 
   if (Lex.getCode() != tgtok::Id)
     return TokError("expected identifier after multiclass for name");
   std::string Name = Lex.getCurStrVal();
if (Lex.getCode() != tgtok::l_brace) {
     // A multiclass body introduces a new scope for local variables.
     TGLocalVarScope *MulticlassScope = PushLocalScope();
 
     while (Lex.getCode() != tgtok::r_brace) {
       switch (Lex.getCode()) {
       default:
         return TokError("expected 'assert', 'def', 'defm', 'defvar', "
                         "'foreach', 'if', or 'let' in multiclass body");
-      case tgtok::Assert:
-        return TokError("an assert statement in a multiclass is not yet supported");
 
+      case tgtok::Assert:
       case tgtok::Def:
       case tgtok::Defm:
       case tgtok::Defvar:
       case tgtok::Foreach:
       case tgtok::If:
       case tgtok::Let:
         if (ParseObject(CurMultiClass))
           return true;
         break;
       }
     }
     Lex.Lex();  // eat the '}'.
 
     // If we have a semicolon, print a gentle error.
     SMLoc SemiLoc = Lex.getLoc();

 ///   Object ::= Defset
 ///   Object ::= Defvar
 ///   Object ::= Assert
 bool TGParser::ParseObject(MultiClass *MC) {
   switch (Lex.getCode()) {
   default:
     return TokError(
                "Expected assert, class, def, defm, defset, foreach, if, or let");
-  case tgtok::Assert:  return ParseAssert(MC, nullptr);
+  case tgtok::Assert:  return ParseAssert(MC);
   case tgtok::Def:     return ParseDef(MC);
   case tgtok::Defm:    return ParseDefm(MC);
   case tgtok::Defvar:  return ParseDefvar();
   case tgtok::Foreach: return ParseForeach(MC);
   case tgtok::If:      return ParseIf(MC);
   case tgtok::Let:     return ParseTopLevelLet(MC);
   case tgtok::Defset:
     if (MC)

llvm/test/TableGen/assert.td

 // CHECK: note: cube of 9
 
 class Cube<int n> {
   int result = !mul(n, n, n);
 }
 
 assert !eq(Cube<9>.result, 81), "cube of 9 should be 729";
 
+// CHECK: assertion failed
+// CHECK: note: foreach i cannot be 2
+// CHECK-NOT: note: foreach i cannot be 2
+
+foreach i = 1...3 in {
+  assert !ne(i, 2), "foreach i cannot be 2";
+  def bar_ # i;
+}
+
 // Test the assert statement in a record definition.
 
 // CHECK: assertion failed
 // CHECK-NOT: primary first name is not "Grace"
 // CHECK: primary first name is not "Grack"
 // CHECK: assertion failed
 // CHECK: foo field should be
 

 
 def Rec32 {
   string Name = Person<"Margaret Heafield Hamilton", 25>.Name;
   int Age = Person<"Margaret Heafield Hamilton", 0>.Age;
 }
 
 // Test the assert statement in a multiclass.
 
+// CHECK: assertion failed
+// CHECK: note: MC1 id string is too long
+// CHECK: assertion failed
+// CHECK: note: MC1 seq is too high
+
+multiclass MC1<string id, int seq> {
+  assert !le(!size(id), 5), "MC1 id string is too long";
+  assert !le(seq, 999999), "MC1 seq is too high";
+
+  def _mc1 {
+    string ID = id;
+    int Seq = seq;
+  }
+}
+
+defm Rec40 : MC1<"ILISP", 999>;
+defm Rec41 : MC1<"ILISPX", 999>;
+defm Rec42 : MC1<"ILISP", 999999999>;
+
+// CHECK: assertion failed
+// CHECK: note: MC2 phrase must be secret: secrex code
+
+multiclass MC2<string phr> {
+  assert !eq(!substr(phr, 0, 6), "secret"), "MC2 phrase must be secret: " # phr;
+
+  def _mc2 {
+    string phrase = phr;
+  }
+}
+
+multiclass MC3<string phr> {
+  defm _mc3 : MC2<phr>;
+}
+
+defm Rec43 : MC3<"secrex code">;
+
+// CHECK: assertion failed
+// CHECK: note: MC2 phrase must be secret: xecret code
+
+multiclass MC4<string phr> : MC2<phr> {
+  def _def;
+}
+
+defm Rec44 : MC4<"xecret code">;