<div dir="ltr">+.. code-block:: llvm<br><div><br></div><div style>This should be a `c++` code block, since this is c++ code (this determines how it is syntax-highlighted).</div><div style><br></div><div style>+  int DIndex = SP::getNamedOperandIdx(SP::XNORrr, SP::OpName::d);     // => -1<br>
</div><div style><br></div><div style>Does it even make sense to return -1 for "not found"?</div><div style><br></div><div style>+///   for looking up the operand index for an instruction, given a vlue from<br></div>
<div style><br></div><div style>Spelling: vlue</div><div style><br></div><div style>+    std::map<unsigned, unsigned> OpList = i->first;<br></div><div style><br></div><div style>Do you really need to make a copy here?</div>
<div style><br></div><div style><div>+    std::map<unsigned, unsigned> OpList = i->first;</div><div>+    std::vector<std::string> OpcodeList = i->second;</div><div><br></div><div style>Same.</div><div style>
<br></div><div style><div>+  for (std::map<std::string, unsigned>::iterator i = Operands.begin(),</div><div>+                                             e = Operands.end();</div><div>+                                             i != e; ++i) {</div>
<div>+    OS << "  " << i->first << " = " << i->second << ",\n";</div><div>+  }</div><div><br></div><div style>The formatting of this looks really awkward. Regardless, you can make this more readable with a typedef:</div>
<div style><br></div><div style><div>typedef std::map<std::string, unsigned>::iterator Iter;</div><div>for (Iter i = Operands.begin(), e = Operands.end(); i != e; ++i)</div><div>  OS << "  " << i->first << " = " << i->second << ",\n";</div>
<div><br></div><div><div>+    std::map<unsigned, unsigned> OpList;</div><div>+    for (unsigned j = 0, je = Inst->Operands.size(); j != je; ++j) {</div><div>+      CGIOperandList::OperandInfo Info = Inst->Operands[j];</div>
<div>+      std::string Name =  Info.Name;</div><div>+      if (Operands.count(Name) == 0) {</div><div>+        Operands[Name] = NumOperands++;</div><div>+      }</div><div>+      unsigned OperandId = Operands[Name];</div>
<div>+      OpList[OperandId] = Info.MIOperandNo;</div><div>+    }</div></div><div><br></div><div style>Do you really need to make a copy of Name? Also, Info is really not cheap to copy (at least 4 std::string and a vector<bool>). Unnecessary copies seems to be a recurring problem in this patch; please go through the entire patch and ensure that you are not unnecessarily making copies.</div>
<div><br></div><div style>Also, You're doing 2-3 separate map lookups for the same element in the inner loop here (are we smart enough to optimize that away? I would be surprised if we were).</div><div style><br></div>
<div style><div>+  for (unsigned i = 0, e = NumberedInstructions.size(), NumOperands = 0;</div><div>+                                                                  i != e; ++i) {</div><div>+    const CodeGenInstruction *Inst = NumberedInstructions[i];</div>
<div>+    if (!Inst->TheDef->getValueAsBit("UseNamedOperandTable")) {</div><div>+      continue;</div><div>+    }</div><div>+    std::map<unsigned, unsigned> OpList;</div><div>+    for (unsigned j = 0, je = Inst->Operands.size(); j != je; ++j) {</div>
<div>+      CGIOperandList::OperandInfo Info = Inst->Operands[j];</div><div>+      std::string Name =  Info.Name;</div><div>+      if (Operands.count(Name) == 0) {</div><div>+        Operands[Name] = NumOperands++;</div>
<div>+      }</div><div>+      unsigned OperandId = Operands[Name];</div><div>+      OpList[OperandId] = Info.MIOperandNo;</div><div>+    }</div><div>+    OperandMap[OpList].push_back(Namespace + "::" + Inst->TheDef->getName());</div>
<div>+  }</div></div><div style><br></div><div style>Can you move this into a helper function?</div><div style><br></div><div style><div>+  unsigned TableIndex = 0;</div><div>+  for (OpNameMapTy::iterator i = OperandMap.begin(), e = OperandMap.end();</div>
<div>+                                                     i != e; ++i, ++TableIndex) {</div><div>+    std::map<unsigned, unsigned> OpList = i->first;</div><div>+    std::vector<std::string> OpcodeList = i->second;</div>
<div>+</div><div>+    for (unsigned ii = 0, ie = OpcodeList.size(); ii != ie; ++ii)</div><div>+      OS << "  case " << OpcodeList[ii] << ":\n";</div><div>+</div><div>+    OS << "    return OperandMap[" << TableIndex << "][NamedIdx];\n";</div>
<div>+  }</div><div><br></div><div style>The ++TableIndex is kind of hidden in the loop header, can you just do TableIndex++ as you output it?</div></div><div style><br></div></div></div></div><div style>-- Sean Silva</div>
</div>