TableGen: Explicitly check whether a record has been resolved

[nhaehnle]

Summary

There are various places where resolving and constant folds can
get stuck, especially around casts. We don't always signal an
error for those, because in many cases they can legitimately
occur without being an error in the "untaken branch" of an !if.

Change-Id: I3befc0e4234c8e6cc61190504702918c9f29ce5c

[tra]

Few minor nits/suggestions. LGTM otherwise.

lib/TableGen/TGParser.cpp

75 ↗

(On Diff #135859)

Nit. HasValue would be more descriptive. OK is too generic. HasValue gives a hint what exactly do we consider to be OK.

84 ↗

(On Diff #135859)

This may be my personal preference, but I find it easier to grasp expressions with smaller number of negations. E.g. for me if(!(Ok || Bit->isConcrete())) is easier for to understand without having to stop and think about it.

100–104 ↗

(On Diff #135859)

I'd fold it into bool OK = isa<BitsInit>(V)) ? checkBitsConcrete(R, RV) : V->isConcrete();

[nhaehnle]

Address style issues, will commit later.

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

public:
   Init(const Init &) = delete;
   Init &operator=(const Init &) = delete;
   virtual ~Init() = default;
 
   /// This virtual method should be overridden by values that may
   /// not be completely specified yet.
   virtual bool isComplete() const { return true; }
 
+  /// Is this a concrete and fully resolved value without any references or
+  /// stuck operations? Unset values are concrete.
+  virtual bool isConcrete() const { return false; }
+
   /// Print out this value.
   void print(raw_ostream &OS) const { OS << getAsString(); }
 
   /// Convert this value to a string form.
   virtual std::string getAsString() const = 0;
   /// Convert this value to a string form,
   /// without adding quote markers.  This primaruly affects
   /// StringInits where we will not surround the string value with
public:
   Init *getCastTo(RecTy *Ty) const override;
   Init *convertInitializerTo(RecTy *Ty) const override;
 
   Init *getBit(unsigned Bit) const override {
     return const_cast<UnsetInit*>(this);
   }
 
   bool isComplete() const override { return false; }
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override { return "?"; }
 };
 
 /// 'true'/'false' - Represent a concrete initializer for a bit.
 class BitInit final : public TypedInit {
   bool Value;
 
   explicit BitInit(bool V) : TypedInit(IK_BitInit, BitRecTy::get()), Value(V) {}
public:
 
   Init *convertInitializerTo(RecTy *Ty) const override;
 
   Init *getBit(unsigned Bit) const override {
     assert(Bit < 1 && "Bit index out of range!");
     return const_cast<BitInit*>(this);
   }
 
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override { return Value ? "1" : "0"; }
 };
 
 /// '{ a, b, c }' - Represents an initializer for a BitsRecTy value.
 /// It contains a vector of bits, whose size is determined by the type.
 class BitsInit final : public TypedInit, public FoldingSetNode,
                        public TrailingObjects<BitsInit, Init *> {
   unsigned NumBits;
public:
   }
 
   bool allInComplete() const {
     for (unsigned i = 0; i != getNumBits(); ++i)
       if (getBit(i)->isComplete()) return false;
     return true;
   }
 
+  bool isConcrete() const override;
   std::string getAsString() const override;
 
   Init *resolveReferences(Resolver &R) const override;
 
   Init *getBit(unsigned Bit) const override {
     assert(Bit < NumBits && "Bit index out of range!");
     return getTrailingObjects<Init *>()[Bit];
   }
public:
 
   static IntInit *get(int64_t V);
 
   int64_t getValue() const { return Value; }
 
   Init *convertInitializerTo(RecTy *Ty) const override;
   Init *convertInitializerBitRange(ArrayRef<unsigned> Bits) const override;
 
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override;
 
   Init *getBit(unsigned Bit) const override {
     return BitInit::get((Value & (1ULL << Bit)) != 0);
   }
 };
 
 /// "foo" - Represent an initialization by a string value.
public:
   }
 
   static StringInit *get(StringRef);
 
   StringRef getValue() const { return Value; }
 
   Init *convertInitializerTo(RecTy *Ty) const override;
 
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override { return "\"" + Value.str() + "\""; }
 
   std::string getAsUnquotedString() const override { return Value; }
 
   Init *getBit(unsigned Bit) const override {
     llvm_unreachable("Illegal bit reference off string");
   }
 };
public:
   }
 
   static CodeInit *get(StringRef);
 
   StringRef getValue() const { return Value; }
 
   Init *convertInitializerTo(RecTy *Ty) const override;
 
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override {
     return "[{" + Value.str() + "}]";
   }
 
   std::string getAsUnquotedString() const override { return Value; }
 
   Init *getBit(unsigned Bit) const override {
     llvm_unreachable("Illegal bit reference off string");
public:
 
   /// This method is used by classes that refer to other
   /// variables which may not be defined at the time they expression is formed.
   /// If a value is set for the variable later, this method will be called on
   /// users of the value to allow the value to propagate out.
   ///
   Init *resolveReferences(Resolver &R) const override;
 
+  bool isConcrete() const override;
   std::string getAsString() const override;
 
   ArrayRef<Init*> getValues() const {
     return makeArrayRef(getTrailingObjects<Init *>(), NumValues);
   }
 
   const_iterator begin() const { return getTrailingObjects<Init *>(); }
   const_iterator end  () const { return begin() + NumValues; }
public:
   Init *convertInitializerTo(RecTy *Ty) const override;
 
   Record *getDef() const { return Def; }
 
   //virtual Init *convertInitializerBitRange(ArrayRef<unsigned> Bits);
 
   RecTy *getFieldType(StringInit *FieldName) const override;
 
+  bool isConcrete() const override { return true; }
   std::string getAsString() const override;
 
   Init *getBit(unsigned Bit) const override {
     llvm_unreachable("Illegal bit reference off def");
   }
 };
 
 /// X.Y - Represent a reference to a subfield of a variable
public:
   }
 
   ArrayRef<StringInit *> getArgNames() const {
     return makeArrayRef(getTrailingObjects<StringInit *>(), NumArgNames);
   }
 
   Init *resolveReferences(Resolver &R) const override;
 
+  bool isConcrete() const override;
   std::string getAsString() const override;
 
   using const_arg_iterator = SmallVectorImpl<Init*>::const_iterator;
   using const_name_iterator = SmallVectorImpl<StringInit*>::const_iterator;
 
   inline const_arg_iterator  arg_begin() const { return getArgs().begin(); }
   inline const_arg_iterator  arg_end  () const { return getArgs().end(); }
 

llvm/trunk/lib/TableGen/Record.cpp

BitsInit::convertInitializerBitRange(ArrayRef<unsigned> Bits) const {
   for (unsigned i = 0, e = Bits.size(); i != e; ++i) {
     if (Bits[i] >= getNumBits())
       return nullptr;
     NewBits[i] = getBit(Bits[i]);
   }
   return BitsInit::get(NewBits);
 }
 
+bool BitsInit::isConcrete() const {
+  for (unsigned i = 0, e = getNumBits(); i != e; ++i) {
+    if (!getBit(i)->isConcrete())
+      return false;
+  }
+  return true;
+}
+
 std::string BitsInit::getAsString() const {
   std::string Result = "{ ";
   for (unsigned i = 0, e = getNumBits(); i != e; ++i) {
     if (i) Result += ", ";
     if (Init *Bit = getBit(e-i-1))
       Result += Bit->getAsString();
     else
       Result += "*";
for (Init *CurElt : getValues()) {
     Resolved.push_back(E);
   }
 
   if (Changed)
     return ListInit::get(Resolved, getElementType());
   return const_cast<ListInit *>(this);
 }
 
+bool ListInit::isConcrete() const {
+  for (Init *Element : *this) {
+    if (!Element->isConcrete())
+      return false;
+  }
+  return true;
+}
+
 std::string ListInit::getAsString() const {
   std::string Result = "[";
   const char *sep = "";
   for (Init *Element : *this) {
     Result += sep;
     sep = ", ";
     Result += Element->getAsString();
   }
Init *DagInit::resolveReferences(Resolver &R) const {
 
   Init *Op = Val->resolveReferences(R);
   if (Op != Val || ArgsChanged)
     return DagInit::get(Op, ValName, NewArgs, getArgNames());
 
   return const_cast<DagInit *>(this);
 }
 
+bool DagInit::isConcrete() const {
+  if (!Val->isConcrete())
+    return false;
+  for (const Init *Elt : getArgs()) {
+    if (!Elt->isConcrete())
+      return false;
+  }
+  return true;
+}
+
 std::string DagInit::getAsString() const {
   std::string Result = "(" + Val->getAsString();
   if (ValName)
     Result += ":" + ValName->getAsUnquotedString();
   if (!arg_empty()) {
     Result += " " + getArg(0)->getAsString();
     if (getArgName(0)) Result += ":$" + getArgName(0)->getAsUnquotedString();
     for (unsigned i = 1, e = getNumArgs(); i != e; ++i) {

llvm/trunk/lib/TableGen/TGParser.cpp

LLVM_DUMP_METHOD void SubMultiClassReference::dump() const {
   errs() << "Template args:\n";
   for (Init *TA : TemplateArgs)
     TA->dump();
 }
 #endif
 
 } // end namespace llvm
 
+static bool checkBitsConcrete(Record &R, const RecordVal &RV) {
+  BitsInit *BV = cast<BitsInit>(RV.getValue());
+  for (unsigned i = 0, e = BV->getNumBits(); i != e; ++i) {
+    Init *Bit = BV->getBit(i);
+    bool IsReference = false;
+    if (auto VBI = dyn_cast<VarBitInit>(Bit)) {
+      if (auto VI = dyn_cast<VarInit>(VBI->getBitVar())) {
+        if (R.getValue(VI->getName()))
+          IsReference = true;
+      }
+    } else if (isa<VarInit>(Bit)) {
+      IsReference = true;
+    }
+    if (!(IsReference || Bit->isConcrete()))
+      return false;
+  }
+  return true;
+}
+
+static void checkConcrete(Record &R) {
+  for (const RecordVal &RV : R.getValues()) {
+    // HACK: Disable this check for variables declared with 'field'. This is
+    // done merely because existing targets have legitimate cases of
+    // non-concrete variables in helper defs. Ideally, we'd introduce a
+    // 'maybe' or 'optional' modifier instead of this.
+    if (RV.getPrefix())
+      continue;
+
+    if (Init *V = RV.getValue()) {
+      bool Ok = isa<BitsInit>(V) ? checkBitsConcrete(R, RV) : V->isConcrete();
+      if (!Ok) {
+        PrintError(R.getLoc(),
+                   Twine("Initializer of '") + RV.getNameInitAsString() +
+                   "' in '" + R.getNameInitAsString() +
+                   "' could not be fully resolved: " +
+                   RV.getValue()->getAsString());
+      }
+    }
+  }
+}
+
 bool TGParser::AddValue(Record *CurRec, SMLoc Loc, const RecordVal &RV) {
   if (!CurRec)
     CurRec = &CurMultiClass->Rec;
 
   if (RecordVal *ERV = CurRec->getValue(RV.getNameInit())) {
     // The value already exists in the class, treat this as a set.
     if (ERV->setValue(RV.getValue()))
       return Error(Loc, "New definition of '" + RV.getName() + "' of type '" +
if (!IterRec->isAnonymous())
       return Error(Loc, "def already exists: " +IterRec->getNameInitAsString());
 
     IterRec->setName(GetNewAnonymousName());
   }
 
   Record *IterRecSave = IterRec.get(); // Keep a copy before release.
   Records.addDef(std::move(IterRec));
   IterRecSave->resolveReferences();
+  checkConcrete(*IterRecSave);
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Parser Code
 //===----------------------------------------------------------------------===//
 
 /// isObjectStart - Return true if this is a valid first token for an Object.
for (const auto &Proto : CurMultiClass->DefPrototypes)
       if (Proto->getNameInit() == CurRec->getNameInit())
         return Error(DefLoc, "def '" + CurRec->getNameInitAsString() +
                      "' already defined in this multiclass!");
     CurMultiClass->DefPrototypes.push_back(std::move(CurRecOwner));
   } else if (ParseObjectBody(CurRec)) {
     return true;
   }
 
-  if (!CurMultiClass)  // Def's in multiclasses aren't really defs.
+  if (!CurMultiClass) { // Def's in multiclasses aren't really defs.
     // See Record::setName().  This resolve step will see any new name
     // for the def that might have been created when resolving
     // inheritance, values and arguments above.
     CurRec->resolveReferences();
+    if (Loops.empty())
+      checkConcrete(*CurRec);
+  }
 
   // If ObjectBody has template arguments, it's an error.
   assert(CurRec->getTemplateArgs().empty() && "How'd this get template args?");
 
   if (CurMultiClass) {
     // Copy the template arguments for the multiclass into the def.
     for (Init *TArg : CurMultiClass->Rec.getTemplateArgs()) {
       const RecordVal *RV = CurMultiClass->Rec.getValue(TArg);
while (true) {
       }
 
       if (Lex.getCode() != tgtok::comma) break;
       Lex.Lex(); // eat ','.
       SubClass = ParseSubClassReference(nullptr, false);
     }
   }
 
-  if (!CurMultiClass)
-    for (Record *CurRec : NewRecDefs)
+  if (!CurMultiClass) {
+    for (Record *CurRec : NewRecDefs) {
       // See Record::setName().  This resolve step will see any new
       // name for the def that might have been created when resolving
       // inheritance, values and arguments above.
       CurRec->resolveReferences();
+      checkConcrete(*CurRec);
+    }
+  }
 
   if (Lex.getCode() != tgtok::semi)
     return TokError("expected ';' at end of defm");
   Lex.Lex();
 
   return false;
 }
 

llvm/trunk/test/TableGen/BitOffsetDecoder.td

def baz : Instruction {
     field bits<16> Inst;
     bits<32> factor;
     let Inst{7-0} = 0xDD;
     let Inst{15-8} = factor{11-4}; // offset by 4 + custom decode
     let AsmString = "baz  $factor";
     field bits<16> SoftFail = 0;
     }
 
-def bum : Instruction {
-    let InOperandList = (ins i32imm:$factor);
-    field bits<16> Inst;
-    bits<32> factor;
-    let Inst{7-0} = 0xEE;
-    let Inst{15-8} = !srl(factor,5);
-    let AsmString = "bum  $factor";
-    field bits<16> SoftFail = 0;
-    }
 }
 
-
 // CHECK: tmp = fieldFromInstruction(insn, 8, 7);
 // CHECK: tmp = fieldFromInstruction(insn, 8, 8) << 3;
 // CHECK: tmp |= fieldFromInstruction(insn, 8, 4) << 7;
 // CHECK: tmp |= fieldFromInstruction(insn, 12, 4) << 3;
 // CHECK: tmp = fieldFromInstruction(insn, 8, 8) << 4;

llvm/trunk/test/TableGen/BitsInitOverflow.td

-// RUN: llvm-tblgen %s | FileCheck %s
+// RUN: not llvm-tblgen %s 2>&1 | FileCheck %s
 
 // Check that a large integer is not truncated to a small bit sequence.
 //
-// CHECK: bits<2> X = { !cast<bits<2>>(5){1}, !cast<bits<2>>(5){0} };
+// CHECK: error: Initializer of 'X' in 'anonymous_0' could not be fully resolved:
 
 def {
   bits<2> X = 5;  // bitfield is too small, reject
 }