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

[Paul-C-Anagnostopoulos]

Summary

I recently modified ListInit::convertInitListSlice() to make it consistent with the list slice rules. I added two lines of code and managed to get 50% of them wrong. Further, I enhanced the ListSlices.td test and managed not to detect my own mistake.

This patch fixes the fix. Thanks to @dsanders for help with this.

[dsanders]

LGTM

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

llvm/lib/TableGen/Record.cpp

618–622

I can confirm that with this fix our downstream predicate passes its tests.

llvm/test/TableGen/ListSlices.td

121

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())

124–125

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.

[dsanders]

The patches I mentioned are in D100253

llvm/lib/TableGen/Record.cpp

if (!Changed)
       return const_cast<ListInit*>(this);
     return ListInit::get(Elements, ElementType);
   }
 
   return nullptr;
 }
 
 Init *ListInit::convertInitListSlice(ArrayRef<unsigned> Elements) const {
-  if (Elements.size() == 1)
-    return getElement(0);
+  if (Elements.size() == 1) {
+    if (Elements[0] >= size())
+      return nullptr;
+    return getElement(Elements[0]);
+  }
 
   SmallVector<Init*, 8> Vals;
   Vals.reserve(Elements.size());
   for (unsigned Element : Elements) {
     if (Element >= size())
       return nullptr;
     Vals.push_back(getElement(Element));
   }

llvm/test/TableGen/ListSlices.td

 // RUN: llvm-tblgen %s | FileCheck %s
 // XFAIL: vg_leak
 
 // This file has tests for the list slice suffix.
 
 // Test defvars and literal lists.
 
 // CHECK: def Rec00
 // CHECK:   list<int> ShowVar1 = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
 // CHECK:   int ShowVar2 = 0
 // CHECK:   list<int> ShowVar3 = [2, 3, 4, 5]
-// CHECK:   int ShowVar4 = 0
+// CHECK:   int ShowVar4 = 2
 // CHECK:   list<int> ShowVar5 = [2, 3, 4, 5]
 
 defvar Var1 = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
 defvar Var2 = Var1[0];
 defvar Var3 = Var1[2...5];
 
-defvar Var4 = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9][0];
+defvar Var4 = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9][2];
 defvar Var5 = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9][2...5];
 
 def Rec00 { // Display the defvars.
   list<int> ShowVar1 = Var1;
   int       ShowVar2 = Var2;
   list<int> ShowVar3 = Var3;
   int       ShowVar4 = Var4;
   list<int> ShowVar5 = Var5;

 def Rec07 {
   list<int> SomeValues = Rec06.Values[0, 3...5, 0, 7, 9...6, 0];
 }
 
 // Test a double subscript.
 
 // CHECK: def Rec08
 // CHECK:   list<list<string>> Array = {{.*}}"foo", "bar", "snork"], ["zoo", "quux", "flarp"]]
-// CHECK:   string Snork = "snork"
+// CHECK:   string Flarp = "flarp"
 // CHECK:   list<string> ZooQuux = ["zoo", "quux"]
 
 def Rec08 {
   list<list<string>> Array = [["foo", "bar", "snork"],
                               ["zoo", "quux", "flarp"]];
-  string Snork = Array[0][2];
+  string Flarp = Array[1][2];
   list<string> ZooQuux = Array[1][0...1];
 }
+
+// Test uninitialized list elements.
+
+// CHECK: def Rec09
+// CHECK:   int Zero = ?;
+// CHECK:   list<int> TwoFive = [2, 3, ?, 5];
+// CHECK: def Rec10
+// CHECK:   list<int> TwoFive = [2, 3, ?, 5];
+
+def Rec09 : Class1<[?, ?, 2, 3, ?, 5, ?]>;
+
+def Rec10 {
+  int Zero = Class1<[?, ?, 2, 3, ?, 5, ?]>.Zero;
+  list<int> TwoFive = Class1<[?, ?, 2, 3, ?, 5, ?]>.TwoFive;
+}
+
+// TO-DO: Notice that the first field in Rec10 is not checked. 
+// It is not fully resolved for reasons that need to be investigated.