[TableGen] Fix anonymous record self-reference in foreach and multiclass

[jyyou.tw]

Summary

If we instantiate self-referenced anonymous records in foreach and
multiclass, the NAME value will point to incorrect record.
It's because anonymous name is resolved too early.

This patch adds AnonymousNameInit to represent an anonymous record
name. When instantiating an anonymous record, it will update the
referred name.

[Paul-C-Anagnostopoulos]

Note that the current behavior is specified in the documentation:

"Every class has an implicit template argument named NAME (uppercase), which is bound to the name of the Def or Defm inheriting the class. The value of NAME is undefined if the class is inherited by an anonymous record."

I can't think of a reason why the current behavior is sacred, but the documentation needs to be updated. I'm happy to do that once this revision is pushed. The description of multiclass also needs work as far as NAME is concerned.

I presume this revision passes all the TableGen, mlir, and clang tests.

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/TGParser.cpp

452

This makes another pass over all the record fields just to resolve references to the anonymous name. Is there a way to enhance Record::resolveReferences to do this in the usual resolution pass (line 457).

[lattner]

Looks structurally good to me, but I'm not a domain expert here.

[jyyou.tw]

Another try

[Paul-C-Anagnostopoulos]

Looks good to me.

[jyyou.tw]

About pre-merge check failure, I reran OpenMP tests on local machine but some tests still failed, with or without the change.
It seems that failure is unrelated to the patch. I tried to restart check but had no permission.
What can I do in this situation?

[Paul-C-Anagnostopoulos]

What do you mean that you tried to restart check?

[jyyou.tw]

In D95309#2527914, @Paul-C-Anagnostopoulos wrote:

What do you mean that you tried to restart check?

I clicked the "Restart Build" link in this page https://reviews.llvm.org/harbormaster/build/119083/

[Paul-C-Anagnostopoulos]

I know nothing about restarting builds.

Have you pushed this revision yet? I don't understand what a pre-merge check is.

Perhaps @dblaikie can help us.

[jyyou.tw]

No, I don't have commit access.

[Paul-C-Anagnostopoulos]

Ah, then I don't think that build matters.

Perhaps David will commit it for you. Otherwise I will learn how to do it.

[dblaikie]

In D95309#2528208, @Paul-C-Anagnostopoulos wrote:

Ah, then I don't think that build matters.

Perhaps David will commit it for you. Otherwise I will learn how to do it.

Generally if you have commit access and have approved a patch & the patch author mentions they don't have commit access - you can go ahead and commit it for them.

Basically arc patch DXXXX will pull their patch down, and then you can git commit --amend --author=... to set the author information to the original author (I usually end up having to search for the user on the LLVM mailing list to find their real name and email address (so the author can be "Firstname Lastname <[e-mail]>") rather than their phabricator username if possible) and commit/push that like any other patch. I think some of the details are here: https://llvm.org/docs/DeveloperPolicy.html#commit-messages

[Paul-C-Anagnostopoulos]

Okay, I will attempt to push this patch on Friday.

[Paul-C-Anagnostopoulos]

@dblaikie, I don't use Arcanist. Can I just download the raw diff via Phabricator and then apply it with 'git apply'?

[dblaikie]

In D95309#2529115, @Paul-C-Anagnostopoulos wrote:

@dblaikie, I don't use Arcanist. Can I just download the raw diff via Phabricator and then apply it with 'git apply'?

Sure, whatever works for you :) Guess that way you might have to also manually copy/paste the commit message and title, etc.

[Paul-C-Anagnostopoulos]

I will push this revision on Tuesday.

[dblaikie]

In D95309#2532913, @Paul-C-Anagnostopoulos wrote:

I will push this revision on Tuesday.

Ideally, if you include the "Differential Revision: " line in the commit, PHabricator will automatically detect the commit closes the revision, and update the revision to mention the closing commit, change the status, etc. (don't feel like you have to post a manual "I closed this" comment or anything, if the automatic thing has done the work - unless there's something you'd like to add)

If that doesn't happen for any reason ( such as in this case) please include the hash of the closing commit in a message and set the status of the revision to "closed".

[dblaikie]

In D95309#2532949, @dblaikie wrote:

In D95309#2532913, @Paul-C-Anagnostopoulos wrote:

I will push this revision on Tuesday.

Ideally, if you include the "Differential Revision: " line in the commit, PHabricator will automatically detect the commit closes the revision, and update the revision to mention the closing commit, change the status, etc. (don't feel like you have to post a manual "I closed this" comment or anything, if the automatic thing has done the work - unless there's something you'd like to add)

If that doesn't happen for any reason ( such as in this case) please include the hash of the closing commit in a message and set the status of the revision to "closed".

Oh, sorry - I misread. I thought it said that it had been puished on Tuesday. - ignore all that. i'm sure it'll work out fine :)

[Paul-C-Anagnostopoulos]

I pushed @jyyou.tw 's other revision on Friday and apparently it went swimmingly.

llvm/include/llvm/TableGen/Record.h

enum InitKind : uint8_t {
     IK_FirstOpInit,
     IK_BinOpInit,
     IK_TernOpInit,
     IK_UnOpInit,
     IK_LastOpInit,
     IK_CondOpInit,
     IK_FoldOpInit,
     IK_IsAOpInit,
+    IK_AnonymousNameInit,
     IK_StringInit,
     IK_VarInit,
     IK_VarListElementInit,
     IK_VarBitInit,
     IK_VarDefInit,
     IK_LastTypedInit,
     IK_UnsetInit
   };
public:
   bool isConcrete() const override { return true; }
   std::string getAsString() const override;
 
   Init *getBit(unsigned Bit) const override {
     return BitInit::get((Value & (1ULL << Bit)) != 0);
   }
 };
 
+/// "anonymous_n" - Represent an anonymous record name
+class AnonymousNameInit : public TypedInit {
+  unsigned Value;
+
+  explicit AnonymousNameInit(unsigned V)
+      : TypedInit(IK_AnonymousNameInit, StringRecTy::get()), Value(V) {}
+
+public:
+  AnonymousNameInit(const AnonymousNameInit &) = delete;
+  AnonymousNameInit &operator=(const AnonymousNameInit &) = delete;
+
+  static bool classof(const Init *I) {
+    return I->getKind() == IK_AnonymousNameInit;
+  }
+
+  static AnonymousNameInit *get(unsigned);
+
+  unsigned getValue() const { return Value; }
+
+  StringInit *getNameInit() const;
+
+  std::string getAsString() const override;
+
+  Init *resolveReferences(Resolver &R) const override;
+
+  Init *getBit(unsigned Bit) const override {
+    llvm_unreachable("Illegal bit reference off string");
+  }
+};
+
 /// "foo" - Represent an initialization by a string value.
 class StringInit : public TypedInit {
 public:
   enum StringFormat {
     SF_String, // Format as "text"
     SF_Code,   // Format as [{text}]
   };
 

llvm/lib/TableGen/Record.cpp

for (unsigned i = 0, e = Bits.size(); i != e; ++i) {
     if (Bits[i] >= 64)
       return nullptr;
 
     NewBits[i] = BitInit::get(Value & (INT64_C(1) << Bits[i]));
   }
   return BitsInit::get(NewBits);
 }
 
+AnonymousNameInit *AnonymousNameInit::get(unsigned V) {
+  return new (Allocator) AnonymousNameInit(V);
+}
+
+StringInit *AnonymousNameInit::getNameInit() const {
+  return StringInit::get(getAsString());
+}
+
+std::string AnonymousNameInit::getAsString() const {
+  return "anonymous_" + utostr(Value);
+}
+
+Init *AnonymousNameInit::resolveReferences(Resolver &R) const {
+  if (R.isFinal())
+    return getNameInit();
+  auto *Old = const_cast<Init *>(static_cast<const Init *>(this));
+  auto *New = R.resolve(Old);
+  return New ? New : Old;
+}
+
 StringInit *StringInit::get(StringRef V, StringFormat Fmt) {
   static StringMap<StringInit*, BumpPtrAllocator &> StringPool(Allocator);
   static StringMap<StringInit*, BumpPtrAllocator &> CodePool(Allocator);
 
   if (Fmt == SF_String) {
     auto &Entry = *StringPool.insert(std::make_pair(V, nullptr)).first;
     if (!Entry.second)
       Entry.second = new (Allocator) StringInit(Entry.getKey(), Fmt);
if (isa<StringRecTy>(getType())) {
       if (StringInit *Name = dyn_cast<StringInit>(LHS)) {
         if (!CurRec && !IsFinal)
           break;
         assert(CurRec && "NULL pointer");
         Record *D;
 
         // Self-references are allowed, but their resolution is delayed until
         // the final resolve to ensure that we get the correct type for them.
-        if (Name == CurRec->getNameInit()) {
+        auto *Anonymous = dyn_cast<AnonymousNameInit>(CurRec->getNameInit());
+        if (Name == CurRec->getNameInit() ||
+            (Anonymous && Name == Anonymous->getNameInit())) {
           if (!IsFinal)
             break;
           D = CurRec;
         } else {
           D = CurRec->getRecords().getDef(Name->getValue());
           if (!D) {
             if (IsFinal)
               PrintFatalError(CurRec->getLoc(),
raw_ostream &llvm::operator<<(raw_ostream &OS, const RecordKeeper &RK) {
   for (const auto &D : RK.getDefs())
     OS << "def " << *D.second;
   return OS;
 }
 
 /// GetNewAnonymousName - Generate a unique anonymous name that can be used as
 /// an identifier.
 Init *RecordKeeper::getNewAnonymousName() {
-  return StringInit::get("anonymous_" + utostr(AnonCounter++));
+  return AnonymousNameInit::get(AnonCounter++);
 }
 
 // These functions implement the phase timing facility. Starting a timer
 // when one is already running stops the running one.
 
 void RecordKeeper::startTimer(StringRef Name) {
   if (TimingGroup) {
     if (LastTimer && LastTimer->isRunning()) {

llvm/lib/TableGen/TGParser.cpp

 bool TGParser::addDefOne(std::unique_ptr<Record> Rec) {
   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());
+    auto *OldName = Rec->getNameInit();
+    auto *NewName = Records.getNewAnonymousName();
+
+    MapResolver R(Rec.get());
+    R.set(OldName, NewName);
+    Rec->resolveReferences(R);
   }
 
   Rec->resolveReferences();
   checkConcrete(*Rec);
 
   CheckRecordAsserts(*Rec);
 
   if (!isa<StringInit>(Rec->getNameInit())) {

llvm/test/TableGen/self-reference.td

 // CHECK: def F0 {
 // CHECK:   Fa as_a = F0;
 // CHECK:   Fb as_b = F0;
 // CHECK: }
 // CHECK: def F0x {
 // CHECK:   Fc as_c = F0;
 // CHECK: }
 
+// CHECK: def anonymous_0 {
+// CHECK:   G g = anonymous_0;
+// CHECK: }
+// CHECK: def anonymous_1 {
+// CHECK:   G g = anonymous_1;
+// CHECK: }
+// CHECK: def anonymous_2 {
+// CHECK:   G g = anonymous_2;
+// CHECK: }
+// CHECK: def anonymous_5 {
+// CHECK:   G g = anonymous_5;
+// CHECK: }
+
 def ops;
 
 class A<dag d> {
   dag a = d;
 }
 
 // This type of self-reference is used in various places defining register
 // classes.

 class Fc<Fb x> {
   Fb as_b = x;
 }
 
 def F0 : Fa, Fb<F0>, Fc<F0>;
 def F0x {
   Fc as_c = F0;
 }
+
+// anonymous record self-reference in foreach and multiclass
+class G {
+  G g = !cast<G>(NAME);
+}
+
+foreach _ = [1, 2] in
+  def : G;
+
+multiclass H {
+  def : G;
+}
+
+defm : H;
+defm : H;