TableGen: simplify BitsInit::resolveReferences

[nhaehnle]

Summary

No functional change intended. The removed code has a loop for
recursive resolving, which is superseded by the recursive
resolving done by the Resolver implementations.

Add a test case which was broken by an earlier version of this
change.

Change-Id: Ib208d037b77a8bbb725977f1388601fc984723d8

[tra]

Few nits on the test. LGTM otherwise.

test/TableGen/UnsetBitInit.td

3 ↗

(On Diff #135546)

More details would be helpful here. E.g.: Verify that Inst still contains references to P and Q, instead of '?' which is their current value.

8 ↗

(On Diff #135546)

Adding checks for P and Q here would help to illustrate the behavior the test verifies.

[nhaehnle]

test/TableGen/UnsetBitInit.td

3 ↗

(On Diff #135546)

Done locally.

8 ↗

(On Diff #135546)

Done locally.

[nhaehnle]

Expand on the test.

[nhaehnle]

Add the missing FileCheck to the test.

llvm/trunk/include/llvm/TableGen/Record.h

public:
   /// users of the value to allow the value to propagate out.
   virtual Init *resolveReferences(Resolver &R) const {
     return const_cast<Init *>(this);
   }
 
   /// This method is used to return the initializer for the specified
   /// bit.
   virtual Init *getBit(unsigned Bit) const = 0;
-
-  /// This method is used to retrieve the initializer for bit
-  /// reference. For non-VarBitInit, it simply returns itself.
-  virtual Init *getBitVar() const { return const_cast<Init*>(this); }
-
-  /// This method is used to retrieve the bit number of a bit
-  /// reference. For non-VarBitInit, it simply returns 0.
-  virtual unsigned getBitNum() const { return 0; }
 };
 
 inline raw_ostream &operator<<(raw_ostream &OS, const Init &I) {
   I.print(OS); return OS;
 }
 
 /// This is the common super-class of types that have a specific,
 /// explicit, type.
public:
   static bool classof(const Init *I) {
     return I->getKind() == IK_VarBitInit;
   }
 
   static VarBitInit *get(TypedInit *T, unsigned B);
 
   Init *convertInitializerTo(RecTy *Ty) const override;
 
-  Init *getBitVar() const override { return TI; }
-  unsigned getBitNum() const override { return Bit; }
+  Init *getBitVar() const { return TI; }
+  unsigned getBitNum() const { return Bit; }
 
   std::string getAsString() const override;
   Init *resolveReferences(Resolver &R) const override;
 
   Init *getBit(unsigned B) const override {
     assert(B < 1 && "Bit index out of range!");
     return const_cast<VarBitInit*>(this);
   }

llvm/trunk/lib/TableGen/Record.cpp

for (unsigned i = 0, e = getNumBits(); i != e; ++i) {
     if (Init *Bit = getBit(e-i-1))
       Result += Bit->getAsString();
     else
       Result += "*";
   }
   return Result + " }";
 }
 
-// Fix bit initializer to preserve the behavior that bit reference from a unset
-// bits initializer will resolve into VarBitInit to keep the field name and bit
-// number used in targets with fixed insn length.
-static Init *fixBitInit(const Resolver &R, Init *Before, Init *After) {
-  if (!isa<UnsetInit>(After) || !R.keepUnsetBits())
-    return After;
-  return Before;
-}
-
 // resolveReferences - If there are any field references that refer to fields
 // that have been filled in, we can propagate the values now.
 Init *BitsInit::resolveReferences(Resolver &R) const {
   bool Changed = false;
   SmallVector<Init *, 16> NewBits(getNumBits());
 
-  Init *CachedInit = nullptr;
-  Init *CachedBitVar = nullptr;
-  bool CachedBitVarChanged = false;
+  Init *CachedBitVarRef = nullptr;
+  Init *CachedBitVarResolved = nullptr;
 
   for (unsigned i = 0, e = getNumBits(); i != e; ++i) {
     Init *CurBit = getBit(i);
-    Init *CurBitVar = CurBit->getBitVar();
+    Init *NewBit = CurBit;
 
-    NewBits[i] = CurBit;
-
-    if (CurBitVar == CachedBitVar) {
-      if (CachedBitVarChanged) {
-        Init *Bit = CachedInit->getBit(CurBit->getBitNum());
-        NewBits[i] = fixBitInit(R, CurBit, Bit);
-      }
-      continue;
+    if (VarBitInit *CurBitVar = dyn_cast<VarBitInit>(CurBit)) {
+      if (CurBitVar->getBitVar() != CachedBitVarRef) {
+        CachedBitVarRef = CurBitVar->getBitVar();
+        CachedBitVarResolved = CachedBitVarRef->resolveReferences(R);
       }
-    CachedBitVar = CurBitVar;
-    CachedBitVarChanged = false;
 
-    Init *B;
-    do {
-      B = CurBitVar;
-      CurBitVar = CurBitVar->resolveReferences(R);
-      CachedBitVarChanged |= B != CurBitVar;
-      Changed |= B != CurBitVar;
-    } while (B != CurBitVar);
-    CachedInit = CurBitVar;
-
-    if (CachedBitVarChanged) {
-      Init *Bit = CurBitVar->getBit(CurBit->getBitNum());
-      NewBits[i] = fixBitInit(R, CurBit, Bit);
+      NewBit = CachedBitVarResolved->getBit(CurBitVar->getBitNum());
+    } else {
+      // getBit(0) implicitly converts int and bits<1> values to bit.
+      NewBit = CurBit->resolveReferences(R)->getBit(0);
     }
+
+    if (isa<UnsetInit>(NewBit) && R.keepUnsetBits())
+      NewBit = CurBit;
+    NewBits[i] = NewBit;
+    Changed |= CurBit != NewBit;
   }
 
   if (Changed)
     return BitsInit::get(NewBits);
 
   return const_cast<BitsInit *>(this);
 }
 

llvm/trunk/test/TableGen/UnsetBitInit.td

-// RUN: llvm-tblgen %s
+// RUN: llvm-tblgen %s | FileCheck %s
 // XFAIL: vg_leak
+
+// CHECK: --- Defs ---
+
+// Test that P and Q are not replaced by ?. TableGen's codegen emitter backend
+// relies on keeping variable references like this around to describe the
+// structure of instruction encodings.
+//
+// CHECK: def A {
+// CHECK:   bits<8> Inst = { 1, 1, 1, 1, 1, 1, P, Q };
+// CHECK:   bits<2> src = { ?, ? };
+// CHECK:   bit P = ?;
+// CHECK:   bit Q = ?;
+// CHECK: }
+
+def A {
+  bits<8> Inst;
+  bits<2> src;
+
+  bit P;
+  bit Q;
+
+  let Inst{7-2} = 0x3f;
+  let Inst{1} = P;
+  let Inst{0} = Q;
+
+  let P = src{1};
+  let Q = src{0};
+}
+
 class x {
   field bits<32> A;
 }
 
 class y<bits<2> B> : x {
   let A{21-20} = B;
 }
 
 def z : y<{0,?}>;