[tblgen] Allow !cast to records that don't yet exist during multiclass parsing.

[tra]

Summary

This makes tablegen's behavior more consistent with the way it lazily folds other operators.
E.g. it's perfectly fine to have a binary/ternary ops that can't be folded at the time of multiclass parsing.

[nhaehnle]

I'm a bit suspicious of this change because it introduces the potential for recursion and I think it relies on some implementation detail of how the !cast is re-resolved once the record is finally instantiated.

I suspect the proper way to fix this is to change multiclass M2 slightly:

multiclass M2 {
  defm NAME: M1<NAME # "d0">;
}

... and augment TGParser::ParseIDValue to explicitly handle "NAME" in the if (CurMultiClass) statement.

That said, I haven't tried this, and record name resolution is a complete inconsistent mess. FWIW, my take on it is that NAME should simply be an implicit class/multiclass template argument, but unfortunately a lot of .td files have grown dependent on subtle implementation details, so cleaning this up is tough.

[tra]

In D43519#1014318, @nhaehnle wrote:

I'm a bit suspicious of this change because it introduces the potential for recursion and I think it relies on some implementation detail of how the !cast is re-resolved once the record is finally instantiated.

I've failed to come up with a way to trigger recursion. We do need to instantiate the records in the end and at that point, if there's recursion, we'll eventually end up doing a !cast to a record that does not exist yet and we will fail then. I may be wrong. Can you think of an example?

One way to think of what this patch does is delaying evaluation of !cast inside multiclass as if the arguments of !cast are not foldable yet. E.g. is M2 would pass to M1 a template parameter instead of a constant string. So, we already allow parsing multiclasses with unresolved casts. Whether it's unresolved because the record does not exist or because the name of the record is not known should not matter, IMO.

I suspect the proper way to fix this is to change multiclass M2 slightly:

multiclass M2 {
  defm NAME: M1<NAME # "d0">;
}

... and augment TGParser::ParseIDValue to explicitly handle "NAME" in the if (CurMultiClass) statement.

The NAME resolution is a different issue. For this patch, the key problem is that in M1 the record created by _r1, which _m1 eventually wants to refer to, does not exist until M1 is instantiated.
M2 is just used to trigger the problem. It's easy to work around by just doing top-level defm d1: M1<"d1">;

That said, I haven't tried this, and record name resolution is a complete inconsistent mess. FWIW, my take on it is that NAME should simply be an implicit class/multiclass template argument, but unfortunately a lot of .td files have grown dependent on subtle implementation details, so cleaning this up is tough.

[nhaehnle]

Please see D43656 to see what I mean.

[nhaehnle]

I think I'm partially changing my mind on this. There are some indirect self-references already happening in existing .td files, and after looking at a number of different approaches I'm inclined to add something like your change here, suitably adapted. A side benefit is that the rule in LangIntro about how !cast works becomes simpler and less dependent on the implementation details of variable resolution.

[tra]

This change is no longer needed due to the recent improvements in tablegen made by @nhaehnle.

llvm/include/llvm/TableGen/Record.h

class Record {
   RecordKeeper &TrackedRecords;
 
   DefInit *TheInit = nullptr;
 
   // Unique record ID.
   unsigned ID;
 
   bool IsAnonymous;
+  bool IsMCRecord;  // Record within multiclass.
 
   // Class-instance values can be used by other defs.  For example, Struct<i>
   // is used here as a template argument to another class:
   //
   //   multiclass MultiClass<int i> {
   //     def Def : Class<Struct<i>>;
   //
   // These need to get fully resolved before instantiating any other
   // definitions that use them (e.g. Def).  However, inside a multiclass they
   // can't be immediately resolved so we mark them ResolveFirst to fully
   // resolve them later as soon as the multiclass is instantiated.
   bool ResolveFirst = false;
 
   void init();
   void checkName();
 
 public:
   // Constructs a record.
   explicit Record(Init *N, ArrayRef<SMLoc> locs, RecordKeeper &records,
-                  bool Anonymous = false) :
+                  bool Anonymous = false, bool MCRecord = false) :
     Name(N), Locs(locs.begin(), locs.end()), TrackedRecords(records),
-    ID(LastID++), IsAnonymous(Anonymous) {
+    ID(LastID++), IsAnonymous(Anonymous), IsMCRecord(MCRecord) {
     init();
   }
 
   explicit Record(StringRef N, ArrayRef<SMLoc> locs, RecordKeeper &records,
                   bool Anonymous = false)
     : 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
-  // record. All other fields can be copied normally.
-  Record(const Record &O) :
-    Name(O.Name), Locs(O.Locs), TemplateArgs(O.TemplateArgs),
+  // record. IsMCRecord is reset as it should be set explicitly and we don't
+  // want to propagate it outside of a multiclass. All other fields can be
+  // copied normally.
+  Record(const Record &O)
+      : Name(O.Name), Locs(O.Locs), TemplateArgs(O.TemplateArgs),
         Values(O.Values), SuperClasses(O.SuperClasses),
         TrackedRecords(O.TrackedRecords), ID(LastID++),
-    IsAnonymous(O.IsAnonymous), ResolveFirst(O.ResolveFirst) { }
+        IsAnonymous(O.IsAnonymous), IsMCRecord(false),
+        ResolveFirst(O.ResolveFirst) {}
 
   static unsigned getNewUID() { return LastID++; }
 
   unsigned getID() const { return ID; }
 
   StringRef getName() const { return cast<StringInit>(Name)->getValue(); }
 
   Init *getNameInit() const {
public:
   bool isResolveFirst() const {
     return ResolveFirst;
   }
 
   void setResolveFirst(bool b) {
     ResolveFirst = b;
   }
 
+  bool isMCRecord() const { return IsMCRecord; }
+  void setMCRecord(bool value) { IsMCRecord = value; }
+
   void print(raw_ostream &OS) const;
   void dump() const;
 
   //===--------------------------------------------------------------------===//
   // High-level methods useful to tablegen back-ends
   //
 
   /// Return the initializer for a value with the specified name,

llvm/lib/TableGen/Record.cpp

if (isa<StringRecTy>(getType())) {
 
             return VarInit::get(MCName, RV->getType());
           }
         }
         assert(CurRec && "NULL pointer");
         if (Record *D = (CurRec->getRecords()).getDef(Name->getValue()))
           return DefInit::get(D);
 
+        // Failure to resolve a reference while we're parsing a multiclass is
+        // not fatal. We'll attempt the cast again when we instantiate the
+        // record for real.
+        if (CurRec->isMCRecord())
+          break;
+
         PrintFatalError(CurRec->getLoc(),
                         "Undefined reference:'" + Name->getValue() + "'\n");
       }
 
       if (isa<IntRecTy>(getType())) {
         if (BitsInit *BI = dyn_cast<BitsInit>(LHS)) {
           if (Init *NewInit = BI->convertInitializerTo(IntRecTy::get()))
             return NewInit;

llvm/lib/TableGen/TGParser.cpp

DefName =
                      DefName, StringRecTy::get())->Fold(DefProto, &MC);
   }
 
   // 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);
 
+  // Mark it as a record internal to a multiclass. These make some errors
+  // non-fatal. E.g. when the record uses !cast for a record that does not exist
+  // yet, because it may be instantiated only after we're done parsing the
+  // multiclass.
+  if (CurMultiClass != nullptr)
+    CurRec->setMCRecord(true);
+
   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.

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

+// RUN: not llvm-tblgen %s 2>&1 | FileCheck %s
+// XFAIL: vg_leak
+
+// This test verifies that tablegen does fail if it can't resolve an unresolved
+// !cast() during processing top-level defm.
+
+class A {}
+class B<A a> {
+  A ba = a;
+}
+
+multiclass M0<string s> {
+  // This should work fine.
+  def _m00 : B<!cast<A>(s)>;
+  // CHECK: error: Undefined reference:'d1_r1_no_such_record'
+  def _m01: B<!cast<A>(s#"_no_such_record")>;
+}
+
+multiclass M1<string s> {
+  def _r1 : A;
+  // It would be nice if we could refer to _r1's name without having to pass it
+  // explicitly via 's'.
+  // XCHECK-DAG: note: instantiated from multiclass
+  defm _m1: M0<s # "_r1">;
+}
+
+// CHECK: defm d1: M1
+// CHECK: note: instantiated from multiclass
+// CHECK: defm _m1: M0
+// CHECK: note: instantiated from multiclass
+// CHECK: def _m01: B
+defm d1: M1<"d1">;

llvm/test/TableGen/MultiClass-defm.td

+// RUN: llvm-tblgen %s | FileCheck %s
+// XFAIL: vg_leak
+
+// Some of the things that would be an error in a top-level defm, should be OK
+// within the multiclass. This test verifies that tablegen can handle !cast to a
+// record that does not exist during multiclass parsing, but which does get
+// created by the time the multiclass record is instantiated.
+
+class A {}
+class B<A a> {
+  A ba = a;
+}
+
+multiclass M0<string s> {
+  def _m0 : B<!cast<A>(s)>;
+
+  // Uncomment to test that !cast will eventually fail if the record it refers
+  // to does not exist by the time we instantiate this record from the top
+  // level.
+  //def _m1 : B<!cast<A>(s#"X")>;
+}
+
+multiclass M1<string s> {
+  def _r1 : A;
+  // It would be nice if we could refer to _r1's name without having to pass it
+  // explicitly via 's'.
+  defm _m1: M0<s # "_r1">;
+}
+
+multiclass M2 {
+  // This used to throw an error during multiclass parsing as M1::_r1 is not
+  // instantiated yet when we attempt to instantiare _m1 within the multiclass.
+  defm NAME: M1<"d0">;
+}
+defm d0: M2;
+// CHECK-LABEL: def d0_m1_m0
+// CHECK: A ba = d0_r1;
+
+// This always works, because d1_r1 is instantiated before d1_m1 which would
+// attempt to !cast to it.
+defm d1: M1<"d1">;
+// CHECK-LABEL: def d1_m1_m0
+// CHECK: A ba = d1_r1;
+