[tablegen] Recursively evaluate values of variable initializers.

[tra]

Summary

Otherwise we often end up with partially evaluated fields even when
their (evaluated) values are known at the time of the record instantiation.

[tra]

@arsenm Can you take a look?

[tra]

@craig.topper please take a look.

[craig.topper]

Wasn't the point of how this was written before so that we go through a loop in Record::resolveReferencesTo and resolve each field and call setValue. And the VarInit just loops up the value saved by that resolving. Now it looks we by pass those cached results and just recurse all the time. Won't this be a big perf hit?

[tra]

In D43150#1009522, @craig.topper wrote:

Wasn't the point of how this was written before so that we go through a loop in Record::resolveReferencesTo and resolve each field and call setValue. And the VarInit just loops up the value saved by that resolving. Now it looks we by pass those cached results and just recurse all the time. Won't this be a big perf hit?

It should not impact performance much. If we've managed to fold the value at some point, then we'll return right away, because there's nothing to fold any more.
If folding failed we will attempt to fold again when the variable is used later. It should not happen often as all currently existing tablegen files apparently manage to provide enough information to fold the records and avoid the problem this patch solves.

Performance impact on my machine using bin/tblgen -I ~/work/llvm/repo/llvm/lib/Target/NVPTX -I ~/work/llvm/repo/llvm/lib/Target -I ~/work/llvm/repo/llvm/include ~/work/llvm/repo/llvm/lib/Target/NVPTX/NVPTX.td -o /dev/null is lost in the noise:
w/o changes: 0.261s-0.275s
with this patch: 0.265s-0.274s

Most of the time will be spent in the back-ends and that part is not affected by the patch.

[tra]

ping.

[nhaehnle]

I believe the change is correct, although the change D43564 should be doing the same thing with caching on top of it.

If this is urgent we could take it and resolve the conflicts, otherwise I'd prefer if we went with my changes if you're okay with them, but keep your test case around.

[tra]

In D43150#1014304, @nhaehnle wrote:

I believe the change is correct, although the change D43564 should be doing the same thing with caching on top of it.

If this is urgent we could take it and resolve the conflicts, otherwise I'd prefer if we went with my changes if you're okay with them, but keep your test case around.

I have few pending patches depending on this change, so I'd rather land this patch soon. The conflict will be easy to resolve -- just replace the changes to VarInit::resolveReferences with your version.
D43564 is currently the last patch in your set, which may take some time until it lands. If you can rebase D43564 to the bottom of your stack (and incorporate this test), I'd be OK going with it.

[tra]

The patch is no longer needed after recent tablegen improvements by @nhaehnle.

llvm/lib/TableGen/Record.cpp

if (const RecordVal *Val = R.getValue(VarName)) {
         return I;
       return nullptr;
     }
   return nullptr;
 }
 
 Init *VarInit::resolveReferences(Record &R, const RecordVal *RV) const {
   if (RecordVal *Val = R.getValue(VarName))
-    if (RV == Val || (!RV && !isa<UnsetInit>(Val->getValue())))
-      return Val->getValue();
+    if (RV == Val || (!RV && !isa<UnsetInit>(Val->getValue()))) {
+      Init *TheInit = Val->getValue();
+      // Prevent recursion if this variable refers to itself.
+      if (TheInit == this || VarName->getAsUnquotedString() == "NAME")
+        return TheInit;
+      // Otherwise resolve all downstream references.
+      return TheInit->resolveReferences(R, RV);
+    }
   return const_cast<VarInit *>(this);
 }
 
 VarBitInit *VarBitInit::get(TypedInit *T, unsigned B) {
   using Key = std::pair<TypedInit *, unsigned>;
   static DenseMap<Key, VarBitInit*> ThePool;
 
   Key TheKey(std::make_pair(T, B));

llvm/test/TableGen/var-eval.td

+// RUN: llvm-tblgen %s | FileCheck %s
+
+// Make sure that tablegen evaluates all values of the record fields.
+def r0;
+def r1;
+class A {
+  dag a = (r0 r1);
+}
+
+class B<int i> : A {
+  dag b0 = (r0 );
+  dag b1 = (r1 );
+  dag b2 = !if(i, b1, b0);
+
+  let a = b2;
+  dag b = b2;
+}
+
+// CHECK-LABEL: def b
+def b : B<1>;
+// both a and b should be evaluated.
+// CHECK: dag a = (r1);
+// CHECK: dag b = (r1);
+
+// NAME is a special case for the field evaluation. Make sure we don't recurse
+// infinitely and that NAME has the right value.
+multiclass M {
+  def NAME : B<0>;
+  def NAME # "a" : B<1>;
+}
+
+defm m : M;
+// CHECK-LABEL: def m
+// CHECK: dag a = (r0);
+// CHECK: dag b = (r0);
+// CHECK: string NAME = "m"
+
+// CHECK-LABEL: def ma
+// CHECK: dag a = (r1);
+// CHECK: dag b = (r1);
+// CHECK: string NAME = "m"