Handle tied sub-operands in AsmMatcherEmitter

The problem this patch addresses is the handling of register tie
constraints in AsmMatcherEmitter, where one operand is tied to a
sub-operand of another operand.  The typical scenario for this to
happen is the tie between the "write-back" register of a pre-inc
instruction, and the base register sub-operand of the memory address
operand of that instruction.

The current AsmMatcherEmitter code attempts to handle tied
operands by emitting the operand as usual first, and emitting
a CVT_Tied node when handling the second (tied) operand.  However,
this really only works correctly if the tied operand does not
have sub-operands (and isn't a sub-operand itself).  Under those
circumstances, a wrong MC operand list is generated.

In discussions with Jim Grosbach, it turned out that the MC operand
list really ought not to contain tied operands in the first place;
instead, it ought to consist of exactly those operands that are
named in the AsmString.  However, getting there requires significant
rework of (some) targets.

This patch fixes the immediate problem, and at the same time makes
one (small) step in the direction of the long-term solution, by
implementing two changes:

1. Restricts the AsmMatcherEmitter handling of tied operands to
   apply solely to simple operands (not complex operands or
   sub-operand of such).

This means that at least we don't get silently corrupt MC operand
lists as output.  However, if we do have tied sub-operands, they
would now no longer be handled at all, except for:

2. If we have an operand that does not occur in the AsmString,
   and also isn't handled as tied operand, simply emit a dummy
   MC operand (constant 0).

This works as long as target code never attempts to access
MC operands that do no not occur in the AsmString (and are
not tied simple operands), which happens to be the case for
all targets where this situation can occur (ARM and PowerPC).

[ Note that this change means that many of the ARM custom
  converters are now superfluous, since the implement the
  same "hack" now performed already by common code. ]

Longer term, we ought to fix targets to never access *any*
MC operand that does not occur in the AsmString (including
tied simple operands), and then finally completely remove
all such operands from the MC operand list.

Patch approved by Jim Grosbach.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@180677 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/utils/TableGen/AsmMatcherEmitter.cpp b/utils/TableGen/AsmMatcherEmitter.cpp
index 65d78593f06acbeabd91954aca1516e127aafac8..218af219a8b7878e6ca0b184bea2993db9a49ed1 100644 (file)
--- a/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/utils/TableGen/AsmMatcherEmitter.cpp
@@ -1322,27 +1322,6 @@ void AsmMatcherInfo::buildInfo() {
       if (CGI.TheDef->getValueAsBit("isCodeGenOnly"))
         continue;
 
-      // Validate the operand list to ensure we can handle this instruction.
-      for (unsigned i = 0, e = CGI.Operands.size(); i != e; ++i) {
-        const CGIOperandList::OperandInfo &OI = CGI.Operands[i];
-
-        // Validate tied operands.
-        if (OI.getTiedRegister() != -1) {
-          // If we have a tied operand that consists of multiple MCOperands,
-          // reject it.  We reject aliases and ignore instructions for now.
-          if (OI.MINumOperands != 1) {
-            // FIXME: Should reject these.  The ARM backend hits this with $lane
-            // in a bunch of instructions. The right answer is unclear.
-            DEBUG({
-                errs() << "warning: '" << CGI.TheDef->getName() << "': "
-                     << "ignoring instruction with multi-operand tied operand '"
-                     << OI.Name << "'\n";
-              });
-            continue;
-          }
-        }
-      }
-
       OwningPtr<MatchableInfo> II(new MatchableInfo(CGI));
 
       II->initialize(*this, SingletonRegisters, AsmVariantNo, RegisterPrefix);
@@ -1529,7 +1508,9 @@ buildInstructionOperandReference(MatchableInfo *II,
   // we want to canonicalize to:
   //   "inc $dst"
   // so that we know how to provide the $dst operand when filling in the result.
-  int OITied = Operands[Idx].getTiedRegister();
+  int OITied = -1;
+  if (Operands[Idx].MINumOperands == 1)
+    OITied = Operands[Idx].getTiedRegister();
   if (OITied != -1) {
     // The tied operand index is an MIOperand index, find the operand that
     // contains it.
@@ -1578,7 +1559,9 @@ void MatchableInfo::buildInstructionResultOperands() {
     const CGIOperandList::OperandInfo &OpInfo = ResultInst->Operands[i];
 
     // If this is a tied operand, just copy from the previously handled operand.
-    int TiedOp = OpInfo.getTiedRegister();
+    int TiedOp = -1;
+    if (OpInfo.MINumOperands == 1)
+      TiedOp = OpInfo.getTiedRegister();
     if (TiedOp != -1) {
       ResOperands.push_back(ResOperand::getTiedOp(TiedOp));
       continue;
@@ -1586,10 +1569,15 @@ void MatchableInfo::buildInstructionResultOperands() {
 
     // Find out what operand from the asmparser this MCInst operand comes from.
     int SrcOperand = findAsmOperandNamed(OpInfo.Name);
-    if (OpInfo.Name.empty() || SrcOperand == -1)
-      PrintFatalError(TheDef->getLoc(), "Instruction '" +
-                    TheDef->getName() + "' has operand '" + OpInfo.Name +
-                    "' that doesn't appear in asm string!");
+    if (OpInfo.Name.empty() || SrcOperand == -1) {
+      // This may happen for operands that are tied to a suboperand of a
+      // complex operand.  Simply use a dummy value here; nobody should
+      // use this operand slot.
+      // FIXME: The long term goal is for the MCOperand list to not contain
+      // tied operands at all.
+      ResOperands.push_back(ResOperand::getImmOp(0));
+      continue;
+    }
 
     // Check if the one AsmOperand populates the entire operand.
     unsigned NumOperands = OpInfo.MINumOperands;
@@ -1620,7 +1608,9 @@ void MatchableInfo::buildAliasResultOperands() {
     const CGIOperandList::OperandInfo *OpInfo = &ResultInst->Operands[i];
 
     // If this is a tied operand, just copy from the previously handled operand.
-    int TiedOp = OpInfo->getTiedRegister();
+    int TiedOp = -1;
+    if (OpInfo->MINumOperands == 1)
+      TiedOp = OpInfo->getTiedRegister();
     if (TiedOp != -1) {
       ResOperands.push_back(ResOperand::getTiedOp(TiedOp));
       continue;
@@ -1843,13 +1833,12 @@ static void emitConvertFuncs(CodeGenTarget &Target, StringRef ClassName,
       case MatchableInfo::ResOperand::TiedOperand: {
         // If this operand is tied to a previous one, just copy the MCInst
         // operand from the earlier one.We can only tie single MCOperand values.
-        //assert(OpInfo.MINumOperands == 1 && "Not a singular MCOperand");
+        assert(OpInfo.MINumOperands == 1 && "Not a singular MCOperand");
         unsigned TiedOp = OpInfo.TiedOperandNum;
         assert(i > TiedOp && "Tied operand precedes its target!");
         Signature += "__Tie" + utostr(TiedOp);
         ConversionRow.push_back(CVT_Tied);
         ConversionRow.push_back(TiedOp);
-        // FIXME: Handle the operand number lookup for tied operands.
         break;
       }
       case MatchableInfo::ResOperand::ImmOperand: {