[TableGen] Better error checking for TIED_TO constraints.

[simon_tatham]

Summary

There are quite strong constraints on how you can use the TIED_TO
constraint between MC operands, many of which are currently not
checked until compiler run time.

MachineVerifier enforces that operands can only be tied together in
pairs (no three-way ties), and MachineInstr::tieOperands enforces that
one of the tied operands must be an output operand (def) and the other
must be an input operand (use).

Now we check these at TableGen time, so that if you violate any of
them in a new instruction definition, you find out immediately,
instead of having to wait until you compile something that makes code
generation hit one of those assertions.

Also in this commit, all the error reports in ParseConstraint now
include the name and source location of the def where the problem
happened, so that if you do trigger any of these errors, it's easier
to find the part of your TableGen input where you made the mistake.

The trunk sources already build successfully with this additional
error check, so I think no in-tree target has any of these problems.

[nhaehnle]

Yay for better error messages! One comment about unfortunate variable naming...

utils/TableGen/CodeGenInstruction.cpp

262–269 ↗

(On Diff #171492)

The naming of SrcOp and DestOp should be reversed. It's quite confusing as-is ;)

[simon_tatham]

utils/TableGen/CodeGenInstruction.cpp

262–269 ↗

(On Diff #171492)

Yes, I see what you mean – now that we're enforcing that one of the operands is written and the other is read, it makes particularly little sense to call them 'source' and 'destination' in that order :-) I'll fix it.

[simon_tatham]

OK, here's a second draft which names the variables more sensibly. Perhaps slight overkill, in that I've used LHSOp and RHSOp for the variables ordered as they are in the source constraints string, and then DestOp and SrcOp once they're reordered to (what should be) a def and a use.

[MatzeB]

LGTM. Looks sensible to me, nitpicks below.

I'm not an expert in this area of tablegen though, so maybe wait a few days before committing (unless someone else accepts as well or someone has a complaint).

utils/TableGen/CodeGenInstruction.cpp

255 ↗

(On Diff #171927)

can this be a StringRef or is Name destroyed/changed on the way?

289 ↗

(On Diff #171927)

I would prefer no auto when the type isn't immediately clear just by looking at the line at hand...

[simon_tatham]

Name is rewritten between creation of LHSOp and RHSOp, so you
can't just make both of them into StringRefs because LHSOp would be
invalidated by the change to Name.

But now you point it out, Name doesn't really need to exist, because
instead of extracting a substring from CStr and searching it using
find_first_of, you can just search CStr itself using
find_first_of with the optional start-position argument. Here's a
rewrite that removes Name completely and turns everything into
StringRef.

Also replaced the auto (after looking up what the type was myself :-).

[simon_tatham]

I've just come back to this patch after being distracted for several weeks (sorry about that). I intend to actually commit it shortly, and this time I'll try not to forget :-)

llvm/trunk/test/TableGen/ConstraintChecking.inc

+include "llvm/Target/Target.td"
+
+def TestTarget : Target;
+
+class Encoding : Instruction {
+  field bits<8> Inst;
+}
+
+class TestReg<string name, bits<1> enc> : Register<name, []> {
+    let HWEncoding{15-1} = 0;
+    let HWEncoding{0} = enc;
+}
+
+def R0 : TestReg<"R0", 0>;
+def R1 : TestReg<"R1", 1>;
+def Reg : RegisterClass<"TestTarget", [i32], 32, (sequence "R%d", 0, 1)>;
+
+class TestInstructionWithConstraints<string cstr> : Encoding {
+  dag OutOperandList = (outs Reg:$dest1, Reg:$dest2);
+  dag InOperandList = (ins Reg:$src1, Reg:$src2);
+  string AsmString = "mnemonic $dest1, $dest2, $src1, $src2";
+  string AsmVariantName = "";
+  let Constraints = cstr;
+  field bits<1> dest1;
+  field bits<1> dest2;
+  field bits<1> src1;
+  field bits<1> src2;
+  let Inst{7-4} = 0b1010;
+  let Inst{3} = dest1{0};
+  let Inst{2} = dest2{0};
+  let Inst{1} = src1{0};
+  let Inst{0} = src2{0};
+}

llvm/trunk/test/TableGen/ConstraintChecking1.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Unrecognized constraint '$dest1 ~ $src2' in 'Foo'
+def Foo : TestInstructionWithConstraints<"$dest1 ~ $src2">;

llvm/trunk/test/TableGen/ConstraintChecking2.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Illegal format for @earlyclobber constraint in 'Foo'
+def Foo : TestInstructionWithConstraints<"@earlyclobber ">;

llvm/trunk/test/TableGen/ConstraintChecking3.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// (This is illegal because the '=' has to be surrounded by whitespace)
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Illegal format for tied-to constraint in 'Foo'
+def Foo : TestInstructionWithConstraints<"$dest1=$dest2">;

llvm/trunk/test/TableGen/ConstraintChecking4.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Output operands '$dest1' and '$dest2' of 'Foo' cannot be tied!
+def Foo : TestInstructionWithConstraints<"$dest1 = $dest2">;

llvm/trunk/test/TableGen/ConstraintChecking5.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Input operands '$src1' and '$src2' of 'Foo' cannot be tied!
+def Foo : TestInstructionWithConstraints<"$src1 = $src2">;

llvm/trunk/test/TableGen/ConstraintChecking6.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Operand '$dest1' of 'Foo' cannot have multiple operands tied to it!
+def Foo : TestInstructionWithConstraints<"$dest1 = $src1, $dest1 = $src2">;

llvm/trunk/test/TableGen/ConstraintChecking7.td

+// RUN: not llvm-tblgen -gen-asm-writer -I %p -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+
+include "ConstraintChecking.inc"
+
+// CHECK: [[FILE]]:[[@LINE+1]]:1: error: Operand '$src1' of 'Foo' cannot have multiple constraints!
+def Foo : TestInstructionWithConstraints<"$dest1 = $src1, $dest2 = $src1">;

llvm/trunk/utils/TableGen/CodeGenInstruction.h

public:
       bool isNone() const { return Kind == None; }
       bool isEarlyClobber() const { return Kind == EarlyClobber; }
       bool isTied() const { return Kind == Tied; }
 
       unsigned getTiedOperand() const {
         assert(isTied());
         return OtherTiedOperand;
       }
+
+      bool operator==(const ConstraintInfo &RHS) const {
+        if (Kind != RHS.Kind)
+          return false;
+        if (Kind == Tied && OtherTiedOperand != RHS.OtherTiedOperand)
+          return false;
+        return true;
+      }
+      bool operator!=(const ConstraintInfo &RHS) const { return *this != RHS; }
     };
 
     /// OperandInfo - The information we keep track of for each operand in the
     /// operand list for a tablegen instruction.
     struct OperandInfo {
       /// Rec - The definition this operand is declared as.
       ///
       Record *Rec;

llvm/trunk/utils/TableGen/CodeGenInstruction.cpp

for (unsigned i = 0, e = MIOpInfo->getNumArgs(); i != e; ++i)
     if (MIOpInfo->getArgNameStr(i) == SubOpName)
       return std::make_pair(OpIdx, i);
 
   // Otherwise, didn't find it!
   PrintFatalError(TheDef->getName() + ": unknown suboperand name in '" + Op + "'");
   return std::make_pair(0U, 0U);
 }
 
-static void ParseConstraint(const std::string &CStr, CGIOperandList &Ops) {
+static void ParseConstraint(const std::string &CStr, CGIOperandList &Ops,
+                            Record *Rec) {
   // EARLY_CLOBBER: @early $reg
   std::string::size_type wpos = CStr.find_first_of(" \t");
   std::string::size_type start = CStr.find_first_not_of(" \t");
   std::string Tok = CStr.substr(start, wpos - start);
   if (Tok == "@earlyclobber") {
     std::string Name = CStr.substr(wpos+1);
     wpos = Name.find_first_not_of(" \t");
     if (wpos == std::string::npos)
-      PrintFatalError("Illegal format for @earlyclobber constraint: '" + CStr + "'");
+      PrintFatalError(
+        Rec->getLoc(), "Illegal format for @earlyclobber constraint in '" +
+        Rec->getName() + "': '" + CStr + "'");
     Name = Name.substr(wpos);
     std::pair<unsigned,unsigned> Op = Ops.ParseOperandName(Name, false);
 
     // Build the string for the operand
     if (!Ops[Op.first].Constraints[Op.second].isNone())
-      PrintFatalError("Operand '" + Name + "' cannot have multiple constraints!");
+      PrintFatalError(
+        Rec->getLoc(), "Operand '" + Name + "' of '" + Rec->getName() +
+        "' cannot have multiple constraints!");
     Ops[Op.first].Constraints[Op.second] =
     CGIOperandList::ConstraintInfo::getEarlyClobber();
     return;
   }
 
   // Only other constraint is "TIED_TO" for now.
   std::string::size_type pos = CStr.find_first_of('=');
-  assert(pos != std::string::npos && "Unrecognized constraint");
+  if (pos == std::string::npos)
+    PrintFatalError(
+      Rec->getLoc(), "Unrecognized constraint '" + CStr +
+      "' in '" + Rec->getName() + "'");
   start = CStr.find_first_not_of(" \t");
-  std::string Name = CStr.substr(start, pos - start);
 
   // TIED_TO: $src1 = $dst
-  wpos = Name.find_first_of(" \t");
-  if (wpos == std::string::npos)
-    PrintFatalError("Illegal format for tied-to constraint: '" + CStr + "'");
-  std::string DestOpName = Name.substr(0, wpos);
-  std::pair<unsigned,unsigned> DestOp = Ops.ParseOperandName(DestOpName, false);
+  wpos = CStr.find_first_of(" \t", start);
+  if (wpos == std::string::npos || wpos > pos)
+    PrintFatalError(
+      Rec->getLoc(), "Illegal format for tied-to constraint in '" +
+      Rec->getName() + "': '" + CStr + "'");
+  std::string LHSOpName = StringRef(CStr).substr(start, wpos - start);
+  std::pair<unsigned,unsigned> LHSOp = Ops.ParseOperandName(LHSOpName, false);
 
-  Name = CStr.substr(pos+1);
-  wpos = Name.find_first_not_of(" \t");
+  wpos = CStr.find_first_not_of(" \t", pos + 1);
   if (wpos == std::string::npos)
-    PrintFatalError("Illegal format for tied-to constraint: '" + CStr + "'");
-
-  std::string SrcOpName = Name.substr(wpos);
-  std::pair<unsigned,unsigned> SrcOp = Ops.ParseOperandName(SrcOpName, false);
-  if (SrcOp > DestOp) {
-    std::swap(SrcOp, DestOp);
-    std::swap(SrcOpName, DestOpName);
-  }
+    PrintFatalError(
+      Rec->getLoc(), "Illegal format for tied-to constraint: '" + CStr + "'");
 
-  unsigned FlatOpNo = Ops.getFlattenedOperandNumber(SrcOp);
+  std::string RHSOpName = StringRef(CStr).substr(wpos);
+  std::pair<unsigned,unsigned> RHSOp = Ops.ParseOperandName(RHSOpName, false);
 
-  if (!Ops[DestOp.first].Constraints[DestOp.second].isNone())
-    PrintFatalError("Operand '" + DestOpName +
+  // Sort the operands into order, which should put the output one
+  // first. But keep the original order, for use in diagnostics.
+  bool FirstIsDest = (LHSOp < RHSOp);
+  std::pair<unsigned,unsigned> DestOp = (FirstIsDest ? LHSOp : RHSOp);
+  StringRef DestOpName = (FirstIsDest ? LHSOpName : RHSOpName);
+  std::pair<unsigned,unsigned> SrcOp = (FirstIsDest ? RHSOp : LHSOp);
+  StringRef SrcOpName = (FirstIsDest ? RHSOpName : LHSOpName);
+
+  // Ensure one operand is a def and the other is a use.
+  if (DestOp.first >= Ops.NumDefs)
+    PrintFatalError(
+      Rec->getLoc(), "Input operands '" + LHSOpName + "' and '" + RHSOpName +
+      "' of '" + Rec->getName() + "' cannot be tied!");
+  if (SrcOp.first < Ops.NumDefs)
+    PrintFatalError(
+      Rec->getLoc(), "Output operands '" + LHSOpName + "' and '" + RHSOpName +
+      "' of '" + Rec->getName() + "' cannot be tied!");
+
+  // The constraint has to go on the operand with higher index, i.e.
+  // the source one. Check there isn't another constraint there
+  // already.
+  if (!Ops[SrcOp.first].Constraints[SrcOp.second].isNone())
+    PrintFatalError(
+      Rec->getLoc(), "Operand '" + SrcOpName + "' of '" + Rec->getName() +
       "' cannot have multiple constraints!");
-  Ops[DestOp.first].Constraints[DestOp.second] =
-    CGIOperandList::ConstraintInfo::getTied(FlatOpNo);
+
+  unsigned DestFlatOpNo = Ops.getFlattenedOperandNumber(DestOp);
+  auto NewConstraint = CGIOperandList::ConstraintInfo::getTied(DestFlatOpNo);
+
+  // Check that the earlier operand is not the target of another tie
+  // before making it the target of this one.
+  for (const CGIOperandList::OperandInfo &Op : Ops) {
+    for (unsigned i = 0; i < Op.MINumOperands; i++)
+      if (Op.Constraints[i] == NewConstraint)
+        PrintFatalError(
+          Rec->getLoc(), "Operand '" + DestOpName + "' of '" + Rec->getName() +
+          "' cannot have multiple operands tied to it!");
+  }
+
+  Ops[SrcOp.first].Constraints[SrcOp.second] = NewConstraint;
 }
 
-static void ParseConstraints(const std::string &CStr, CGIOperandList &Ops) {
+static void ParseConstraints(const std::string &CStr, CGIOperandList &Ops,
+                             Record *Rec) {
   if (CStr.empty()) return;
 
   const std::string delims(",");
   std::string::size_type bidx, eidx;
 
   bidx = CStr.find_first_not_of(delims);
   while (bidx != std::string::npos) {
     eidx = CStr.find_first_of(delims, bidx);
     if (eidx == std::string::npos)
       eidx = CStr.length();
 
-    ParseConstraint(CStr.substr(bidx, eidx - bidx), Ops);
+    ParseConstraint(CStr.substr(bidx, eidx - bidx), Ops, Rec);
     bidx = CStr.find_first_not_of(delims, eidx);
   }
 }
 
 void CGIOperandList::ProcessDisableEncoding(std::string DisableEncoding) {
   while (1) {
     std::pair<StringRef, StringRef> P = getToken(DisableEncoding, " ,\t");
     std::string OpName = P.first;
CodeGenInstruction::CodeGenInstruction(Record *R)
   ImplicitDefs = R->getValueAsListOfDefs("Defs");
   ImplicitUses = R->getValueAsListOfDefs("Uses");
 
   // This flag is only inferred from the pattern.
   hasChain = false;
   hasChain_Inferred = false;
 
   // Parse Constraints.
-  ParseConstraints(R->getValueAsString("Constraints"), Operands);
+  ParseConstraints(R->getValueAsString("Constraints"), Operands, R);
 
   // Parse the DisableEncoding field.
   Operands.ProcessDisableEncoding(R->getValueAsString("DisableEncoding"));
 
   // First check for a ComplexDeprecationPredicate.
   if (R->getValue("ComplexDeprecationPredicate")) {
     HasComplexDeprecationPredicate = true;
     DeprecatedReason = R->getValueAsString("ComplexDeprecationPredicate");