[TableGen] Make behavior of list slice suffix consistent across all values

[Paul-C-Anagnostopoulos]

Summary

The list slice suffix with a single index is supposed to produce the specified list element. For example:

[0, 1, 2, 3, 4, 5] [3] => 3

This works fine except when the list is a literal (as above) or a defvar, in which case it currently produces a list of the single element:

[0, 1, 2, 3, 4, 5] [3] => [3]

This revision fixes this bug so that everything is consistent. I can find no TableGen files that depend on the inconsistency. I wrote a more comprehensive test.

[dblaikie]

llvm/lib/TableGen/Record.cpp

618–620

Where's the code that already supports the non-literal case? Is there any chance of collapsing these two cases together?

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/Record.cpp

618–620

It's in TypedInit::convertInitListSlice(). It's sufficiently different that parameterizing a common subfunction would be more complicated than simply repeating the few common-ish lines of code.

[dblaikie]

llvm/lib/TableGen/Record.cpp

618–620

What if convertInitListSlice always returned in the ListInit form, but TGParser.cpp ParseValue checked for length 1 and did the work to turn it into a VarListElementInit or whatnot?

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/Record.cpp

618–620

I assume you're still trying to combine the two convertInitListSlice() functions. But in the loop, the thing that is push_back'ed onto the vector is different in the two functions. And the common case of a single index would build a vector for no reason. I suppose we could build a list of the Inits and then convert to a list of TypeInits, but is that really simpler?

[lattner]

nice testcase!

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/Record.cpp

618–620

Oh wait, my last sentence makes no sense. Anyway, does taking some of the logic out of the two functions really make it simpler overall? I guess I'm still confused about what you're suggesting.

[dblaikie]

llvm/lib/TableGen/Record.cpp

618–620

I assume you're still trying to combine the two convertInitListSlice() functions.

Well, combine the "1 element list becomes an element instead of a list" functionality so it's implemented in one place, yeah. It looks like the "listify this thing" functionality is a bit different and couldn't be unified, but maybe the "1 element list becomes an element instead of a list" part could be unified.

I'd probably be OK with building a vector of 1 element to unify the handling here - unless that proved to be particularly performance prohibitive.

618–620

I assume you're still trying to combine the two convertInitListSlice() functions.

Well, combine the "1 element list becomes an element instead of a list" functionality so it's implemented in one place, yeah. It looks like the "listify this thing" functionality is a bit different and couldn't be unified, but maybe the "1 element list becomes an element instead of a list" part could be unified.

I'd probably be OK with building a vector of 1 element to unify the handling here - unless that proved to be particularly performance prohibitive.

does taking some of the logic out of the two functions really make it simpler overall?

I think so - means updates/changes to the code aren't likely to fix one place and fail to fix the other place, for instance.

But up to you/@lattner - if you find this to be better/easier to maintain this way, that's OK.

[nhaehnle]

The change makes sense to me. Does this need a documentation update?

[Paul-C-Anagnostopoulos]

An update to the TableGen Programmer's Reference was pushed and is waiting for the build to finish.

[Paul-C-Anagnostopoulos]

Regarding simplifying the convertInitListSlice() functions: I will push this revision as it is, then look at the possibility of simplifying. There is also convertInitializerBitRange(), for which there are three variants.

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);
+
   SmallVector<Init*, 8> Vals;
   Vals.reserve(Elements.size());
   for (unsigned Element : Elements) {
     if (Element >= size())
       return nullptr;
     Vals.push_back(getElement(Element));
   }
   return ListInit::get(Vals, getElementType());

llvm/test/TableGen/ListSlices.td

-// RUN: llvm-tblgen %s
+// RUN: llvm-tblgen %s | FileCheck %s
 // XFAIL: vg_leak
 
-def A {
-  list<int> B = [10, 20, 30, 4, 1, 1231, 20];
+// 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:   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 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;
+}
+
+// CHECK: def Rec01
+// CHECK:   int Zero = 0
+// CHECK:   list<int> TwoFive = [2, 3, 4, 5]
+
+def Rec01 {
+  int Zero = Var1[0];
+  list<int> TwoFive = Var1[2...5];
+}
+
+// Test class template arguments.
+
+// CHECK: def Rec02
+// CHECK:   int Zero = 10
+// CHECK:   list<int> TwoFive = [12, 13, 14, 15]
+
+class Class1<list<int> ids> {
+  int Zero = ids[0];
+  list<int> TwoFive = ids[2...5];
+}
+
+def Rec02 : Class1<[10, 11, 12, 13, 14, 15, 16, 17]>;
+
+// Test anonymous record fetches.
+
+// CHECK: def Rec03
+// CHECK:   int Zero = 20
+// CHECK:   list<int> TwoFive = [22, 23, 24, 25]
+
+def Rec03 {
+  int Zero = Class1<[20, 21, 22, 23, 24, 25, 26]>.Zero;
+  list<int> TwoFive = Class1<[20, 21, 22, 23, 24, 25, 26]>.TwoFive;
+}
+
+// Test multiclass template arguments.
+
+// CHECK: def Rec04_MC1
+// CHECK:   int Zero = 30
+// CHECK:   list<int> TwoFive = [32, 33, 34, 35]
+// CHECK: def Rec05_MC1
+// CHECK:   int Zero = 30
+// CHECK:   list<int> TwoFive = [32, 33, 34, 35]
+
+multiclass MC1<list<int> ids> {
+  def _MC1 {
+    int Zero = ids[0];
+    list<int> TwoFive = ids[2...5];
   }
+}
+
+defm Rec04 : MC1<[30, 31, 32, 33, 34, 35, 36]>;
+defm Rec05 : MC1<[30, 31, 32, 33, 34, 35]>;
 
-def B {
-  list<int> X = [10, 20, 30, 4, 1, 1231, 20] [2...4,2,2,0...6];
+// Test taking a complex subset of the list items from another record.
 
-  list<int> Y = X[4,5];
-  int Z = X[4];
+// CHECK: def Rec07
+// CHECK:   list<int> SomeValues = [40, 43, 44, 45, 40, 47, 49, 48, 47, 46, 40]
 
-  list<int> C = A.B[1...4];
+def Rec06 {
+  list<int> Values = [40, 41, 42, 43, 44, 45, 46, 47, 48, 49];
+}
+
+def Rec07 {
+  list<int> SomeValues = Rec06.Values[0, 3...5, 0, 7, 9...6, 0];
+}
 
-  list<list<int>> AA = [X, Y];
+// Test a double subscript.
 
-  int BB = AA[0][1];
+// CHECK: def Rec08
+// CHECK:   list<list<string>> Array = {{.*}}"foo", "bar", "snork"], ["zoo", "quux", "flarp"]]
+// CHECK:   string Snork = "snork"
+// CHECK:   list<string> ZooQuux = ["zoo", "quux"]
+
+def Rec08 {
+  list<list<string>> Array = [["foo", "bar", "snork"],
+                              ["zoo", "quux", "flarp"]];
+  string Snork = Array[0][2];
+  list<string> ZooQuux = Array[1][0...1];
 }