[PATCH] D35998: [TableGen] AsmMatcher: fix OpIdx computation when HasOptionalOperands is true

[Alexandru Guduleasa via Phabricator via llvm-commits]

alexandru.guduleasa updated this revision to Diff 109506.
alexandru.guduleasa added a comment.

We managed to reproduce this using Visual Studio; thank you for your patience.
The issue was that for Tied operand, the OpIdx is an index in Inst, not in the Operands vector.
Since we subtracted 1 for the mnemonic, we had an underflow access in the array.
For Tied Operands, we don't need the array at all since the index is in the Inst, not the Operands.


https://reviews.llvm.org/D35998

Files:
  utils/TableGen/AsmMatcherEmitter.cpp


Index: utils/TableGen/AsmMatcherEmitter.cpp
===================================================================
--- utils/TableGen/AsmMatcherEmitter.cpp
+++ utils/TableGen/AsmMatcherEmitter.cpp
@@ -1847,13 +1847,30 @@
   CvtOS << "  assert(Kind < CVT_NUM_SIGNATURES && \"Invalid signature!\");\n";
   CvtOS << "  const uint8_t *Converter = ConversionTable[Kind];\n";
   if (HasOptionalOperands) {
-    CvtOS << "  unsigned NumDefaults = 0;\n";
+    size_t MaxNumOperands = 0;
+    for (const auto &MI : Infos) {
+      MaxNumOperands = std::max(MaxNumOperands, MI->AsmOperands.size());
+    }
+    CvtOS << "  unsigned DefaultsOffset[" << (MaxNumOperands) << "];\n";
+    CvtOS << "  assert(OptionalOperandsMask.size() == " << (MaxNumOperands)
+          << ");\n";
+    CvtOS << "  for (unsigned i = 0, NumDefaults = 0; i < " << (MaxNumOperands)
+          << "; ++i) {\n";
+    CvtOS << "    DefaultsOffset[i] = NumDefaults;\n";
+    CvtOS << "    NumDefaults += (OptionalOperandsMask[i] ? 1 : 0);\n";
+    CvtOS << "  }\n";
   }
   CvtOS << "  unsigned OpIdx;\n";
   CvtOS << "  Inst.setOpcode(Opcode);\n";
   CvtOS << "  for (const uint8_t *p = Converter; *p; p+= 2) {\n";
   if (HasOptionalOperands) {
-    CvtOS << "    OpIdx = *(p + 1) - NumDefaults;\n";
+    // OpIdx has different semantics for Tied operands and the rest of the
+    // operands. For Tied it is the index in the Inst, therefore we use it
+    // directly. For the rest of the operands, we need to account for the
+    // offset.
+    CvtOS << "    OpIdx = *(p + 1);\n";
+    CvtOS << "    OpIdx -= (*p != CVT_Tied) ? DefaultsOffset[*(p + 1) - 1] : "
+             "0;\n";
   } else {
     CvtOS << "    OpIdx = *(p + 1);\n";
   }
@@ -1988,7 +2005,6 @@
                 << "        " << Op.Class->DefaultMethod << "()"
                 << "->" << Op.Class->RenderMethod << "(Inst, "
                 << OpInfo.MINumOperands << ");\n"
-                << "        ++NumDefaults;\n"
                 << "      } else {\n"
                 << "        static_cast<" << TargetOperandClass
                 << "&>(*Operands[OpIdx])." << Op.Class->RenderMethod


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D35998.109506.patch
Type: text/x-patch
Size: 2130 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170803/d86ee83e/attachment.bin>