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

[Sean Silva]

Hi Tom,

This patch could really use some documentation.

I recommend adding a description of this new TableGen capability to the
appropriate part of <http://llvm.org/docs/WritingAnLLVMBackend.html>
(docs/WritingAnLLVMBackend.rst), or possibly adding a new page like <
http://llvm.org/docs/HowToUseInstrMappings.html> (although that was a
larger patch; most of my comments in the review for that feature are likely
relevant for this patch <
http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/123031>) and linking
to it from WritingAnLLVMBackend . At the very least, please describe the
interface of the generated code (something like "this results in the
generation of a namespace that looks like ..., and a function with the
following signature and name: ..."), and a before/after of how to modify
existing TableGen patterns to take advantage of this functionality. If you
decide to make a separate page, you might find <
http://llvm.org/docs/SphinxQuickstartTemplate.html> helpful.

Your second patch seems to introduce some tricky magic #define's to change
the content of the .inc file. I think those ought to be documented
alongside what I mentioned above.

but may have a different operand index depending on the instruction type.
> It is useful to have a convenient way to look up the operand indices,
> so these bits can be generically set on any instruction.
> v2:
>   - Don't uppercase enum values
>   - Use table compresion to reduce function size
> v3:
>   - Only generate table for instructions with the UseNamedOperandTable
>     bit set.
>

These "patch versioning" remarks in the commit message don't seem to be
really adding much. The review thread is sufficient record of the iteration
on the patch.

+
+  // Operand name -> index mapping
+

Could you maybe give some description of what this does and what data
structures it creates in the .inc file (or cross-reference to such
documentation in docs/)? Also, possibly move the new code into separate
functions/methods (unless it needs to access a ton of local variables).

+  std::map<std::string, unsigned> Operands;
+  OpNameMapTy OperandMap;
+  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());
+  }

I really feel like this computation (which seems to just be initializing
`Operands` and `OperandMap`) ought to be factored out into a method.

+  typedef std::map<std::map<unsigned, unsigned>,
+                   std::vector<std::string> > OpNameMapTy;

This is a pretty complex data type. It deserves a bit of documentation
about what each part means (what does each of the two `unsigned`'s
represent? what does each std::string represent? what is being modeled by
having a vector of strings?)

+    for (std::vector<std::string>::iterator ii = OpcodeList.begin(),
+                                            ie = OpcodeList.end();
+                                            ii != ie; ++ii) {
+      std::string OpName = *ii;
+      OS << "  case " << OpName << ":\n";
+    }

This is really verbose. You should be able to reduce this to two lines by
using a counted loop:

for (unsigned ii = 0, ie = OpcodeList.size() ; ii != ie; ++ii)
  OS << "  case " << OpcodeList[ii] << ":\n";

+    for (unsigned Idx = 0; Idx < Operands.size(); ++Idx) {
+      if (OpList.count(Idx) == 0) {
+        OS << "-1";
+      } else {
+        OS << OpList[Idx];
+      }
+      OS << ", ";
+    }

Don't evaluate `.size()` every time through the loop. Also, although it is
a bit terse, the following loop seems more compact and readable to me:

for (unsigned i = 0, e = Operands.size(); i != e; ++i)
  OS << (OpList.count(Idx) == 0 ? "-1" : OpList[Idx]) << ", ";

Target::getNamedOperandIdx(Target::ADD, Target::OpName::DST)  => 0
> Target::getNamedOperandIdx(Target::ADD, Target::OpName::SRC0) => 1
> Target::getNamedOperandIdx(Target::ADD, Target::OpName::SRC1) => 2
>


> The operand names are case insensitive, so $dst is equivalent to $DST.


In the second patch, it seems like all the values in the namespace
Target::OpName are purely lowercase, such as `src0_sel` in:

-    TII->getOperandIdx(Opcode, R600Operands::SRC0_SEL),
-    TII->getOperandIdx(Opcode, R600Operands::SRC1_SEL),
-    TII->getOperandIdx(Opcode, R600Operands::SRC2_SEL)
+    TII->getOperandIdx(Opcode, AMDGPU::OpName::src0_sel),
+    TII->getOperandIdx(Opcode, AMDGPU::OpName::src1_sel),
+    TII->getOperandIdx(Opcode, AMDGPU::OpName::src2_sel)

I think the commit message (or better yet, documentation in docs/) of the
first patch should indicate the case of the resulting constants (do they
all get lowercased?).

Also, as a side note, the AMDGPU backend doesn't seem to have any content
in <http://llvm.org/docs/CompilerWriterInfo.html>
(docs/CompilerWriterInfo.rst). Would you mind adding some links to the
appropriate manuals?

Thanks,

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