[PATCH] D100247: [TableGen] Fix bug in recent change to ListInit::convertInitListSlice()

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

LGTM

I'll post a patch for the Rec10.Zero fix in a moment



================
Comment at: llvm/lib/TableGen/Record.cpp:618-622
+  if (Elements.size() == 1) {
+    if (Elements[0] >= size())
+      return nullptr;
+    return getElement(Elements[0]);
+  }
----------------
I can confirm that with this fix our downstream predicate passes its tests.


================
Comment at: llvm/test/TableGen/ListSlices.td:121
+  int Zero = Class1<[?, ?, 2, 3, ?, 5, ?]>.Zero;
+  list<int> TwoFive = Class1<[?, ?, 2, 3, ?, 5, ?]>.TwoFive;
+}
----------------
I believe this one resolves because of another bug. After checking the FieldInit::Fold hunk, I looked through the rest of that patch in case there was anything else and we also have these hunks:
```
--- a/llvm/include/llvm/TableGen/Record.h
+++ b/llvm/include/llvm/TableGen/Record.h
@@ -708,6 +708,7 @@ public:
   ///
   Init *resolveReferences(Resolver &R) const override;
 
+  bool isComplete() const override;
   bool isConcrete() const override;
   std::string getAsString() const override;
 
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -652,6 +655,14 @@ Init *ListInit::resolveReferences(Resolver &R) const {
   return const_cast<ListInit *>(this);
 }
 
+bool ListInit::isComplete() const {
+  for (Init *Element : *this) {
+    if (!Element->isComplete())
+      return false;
+  }
+  return true;
+}
+
 bool ListInit::isConcrete() const {
   for (Init *Element : *this) {
     if (!Element->isConcrete())
```
with this but not the FieldInit::Fold hunk, Rec10.TwoFive fails to resolve just like Rec10.Zero does. Without it, ListInit uses Init::isComplete which is always true, ignoring whether the elements themselves are complete. This is inconsistent with BitsInit which does check the individual bits (and appears to have two identical functions doing that isComplete() and allInComplete())


================
Comment at: llvm/test/TableGen/ListSlices.td:124-125
+
+// TO-DO: Notice that the first field in Rec10 is not checked. 
+// It is not fully resolved for reasons that need to be investigated.
----------------
This didn't happen on my machine:
```
def Rec09 {	// Class1
  int Zero = ?;
  list<int> TwoFive = [2, 3, ?, 5];
}
def Rec10 {
  int Zero = ?;
  list<int> TwoFive = [2, 3, ?, 5];
}
```

The reason it works for me is this hunk from our patch that added !iscomplete():
```
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -1921,7 +1937,7 @@ Init *FieldInit::Fold(Record *CurRec) const {
                       FieldName->getAsUnquotedString() + "' of '" +
                       Rec->getAsString() + "' is a forbidden self-reference");
     Init *FieldVal = Def->getValue(FieldName)->getValue();
-    if (FieldVal->isComplete())
+    if (FieldVal->isConcrete())
       return FieldVal;
   }
   return const_cast<FieldInit *>(this);
```
Looking back on it I probably should have upstreamed this even though the rest of the patch wasn't useful to upstream.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100247/new/

https://reviews.llvm.org/D100247