[PATCH 1/2] TableGen: Generate a function for getting operand indices based on their defined names

[Sean Silva]

+.. code-block:: llvm

This should be a `c++` code block, since this is c++ code (this determines
how it is syntax-highlighted).

+  int DIndex = SP::getNamedOperandIdx(SP::XNORrr, SP::OpName::d);     //
=> -1

Does it even make sense to return -1 for "not found"?

+///   for looking up the operand index for an instruction, given a vlue
from

Spelling: vlue

+    std::map<unsigned, unsigned> OpList = i->first;

Do you really need to make a copy here?

+    std::map<unsigned, unsigned> OpList = i->first;
+    std::vector<std::string> OpcodeList = i->second;

Same.

+  for (std::map<std::string, unsigned>::iterator i = Operands.begin(),
+                                             e = Operands.end();
+                                             i != e; ++i) {
+    OS << "  " << i->first << " = " << i->second << ",\n";
+  }

The formatting of this looks really awkward. Regardless, you can make this
more readable with a typedef:

typedef std::map<std::string, unsigned>::iterator Iter;
for (Iter i = Operands.begin(), e = Operands.end(); i != e; ++i)
  OS << "  " << i->first << " = " << i->second << ",\n";

+    std::map<unsigned, unsigned> OpList;
+    for (unsigned j = 0, je = Inst->Operands.size(); j != je; ++j) {
+      CGIOperandList::OperandInfo Info = Inst->Operands[j];
+      std::string Name =  Info.Name;
+      if (Operands.count(Name) == 0) {
+        Operands[Name] = NumOperands++;
+      }
+      unsigned OperandId = Operands[Name];
+      OpList[OperandId] = Info.MIOperandNo;
+    }

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.

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).

+  for (unsigned i = 0, e = NumberedInstructions.size(), NumOperands = 0;
+                                                                  i != e;
++i) {
+    const CodeGenInstruction *Inst = NumberedInstructions[i];
+    if (!Inst->TheDef->getValueAsBit("UseNamedOperandTable")) {
+      continue;
+    }
+    std::map<unsigned, unsigned> OpList;
+    for (unsigned j = 0, je = Inst->Operands.size(); j != je; ++j) {
+      CGIOperandList::OperandInfo Info = Inst->Operands[j];
+      std::string Name =  Info.Name;
+      if (Operands.count(Name) == 0) {
+        Operands[Name] = NumOperands++;
+      }
+      unsigned OperandId = Operands[Name];
+      OpList[OperandId] = Info.MIOperandNo;
+    }
+    OperandMap[OpList].push_back(Namespace + "::" +
Inst->TheDef->getName());
+  }

Can you move this into a helper function?

+  unsigned TableIndex = 0;
+  for (OpNameMapTy::iterator i = OperandMap.begin(), e = OperandMap.end();
+                                                     i != e; ++i,
++TableIndex) {
+    std::map<unsigned, unsigned> OpList = i->first;
+    std::vector<std::string> OpcodeList = i->second;
+
+    for (unsigned ii = 0, ie = OpcodeList.size(); ii != ie; ++ii)
+      OS << "  case " << OpcodeList[ii] << ":\n";
+
+    OS << "    return OperandMap[" << TableIndex << "][NamedIdx];\n";
+  }

The ++TableIndex is kind of hidden in the loop header, can you just do
TableIndex++ as you output it?

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130619/73c615c0/attachment.html>