<div dir="ltr"><div style>Hi Tom,</div><div style><br></div><div style>This patch could really use some documentation.</div><div style><br></div><div style>I recommend adding a description of this new TableGen capability to the appropriate part of <<a href="http://llvm.org/docs/WritingAnLLVMBackend.html">http://llvm.org/docs/WritingAnLLVMBackend.html</a>> (docs/WritingAnLLVMBackend.rst), or possibly adding a new page like <<a href="http://llvm.org/docs/HowToUseInstrMappings.html">http://llvm.org/docs/HowToUseInstrMappings.html</a>> (although that was a larger patch; most of my comments in the review for that feature are likely relevant for this patch <<a href="http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/123031">http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/123031</a>>) 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 <<a href="http://llvm.org/docs/SphinxQuickstartTemplate.html">http://llvm.org/docs/SphinxQuickstartTemplate.html</a>> helpful.</div>
<div style><br></div><div style>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.</div><br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">but may have a different operand index depending on the instruction type.<br>
It is useful to have a convenient way to look up the operand indices,<br>so these bits can be generically set on any instruction.<br>v2:<br> - Don't uppercase enum values<br> - Use table compresion to reduce function size<br>
v3:<br> - Only generate table for instructions with the UseNamedOperandTable<br> bit set.<br></blockquote><div><br></div><div style>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.</div>
<div style><br></div><div style><div><div>+</div><div>+ // Operand name -> index mapping</div><div>+</div></div><div><br></div><div style>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).</div>
<div style><br></div><div style><div>+ std::map<std::string, unsigned> Operands;</div><div>+ OpNameMapTy OperandMap;</div><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><br></div><div style>I really feel like this computation (which seems to just be initializing `Operands` and `OperandMap`) ought to be factored out into a method.</div><div style><br></div><div style><div>
+ typedef std::map<std::map<unsigned, unsigned>,</div><div>+ std::vector<std::string> > OpNameMapTy;</div><div><br></div><div style>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?)</div>
</div></div></div><div><br></div><div>+ for (std::vector<std::string>::iterator ii = OpcodeList.begin(),</div><div>+ ie = OpcodeList.end();</div><div>+ ii != ie; ++ii) {</div>
<div>+ std::string OpName = *ii;</div><div>+ OS << " case " << OpName << ":\n";</div><div>+ }</div><div><br></div><div>This is really verbose. You should be able to reduce this to two lines by using a counted loop:</div>
<div><br></div><div>for (unsigned ii = 0, ie = OpcodeList.size() ; ii != ie; ++ii) </div><div> OS << " case " << OpcodeList[ii] << ":\n";</div><div><br></div><div><div>+ for (unsigned Idx = 0; Idx < Operands.size(); ++Idx) {</div>
<div>+ if (OpList.count(Idx) == 0) {</div><div>+ OS << "-1";</div><div>+ } else {</div><div>+ OS << OpList[Idx];</div><div>+ }</div><div>+ OS << ", ";</div>
<div>+ }</div></div><div><br></div><div style>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:</div><div style><br></div>
<div style><div>for (unsigned i = 0, e = Operands.size(); i != e; ++i)</div><div> OS << (OpList.count(Idx) == 0 ? "-1" : OpList[Idx]) << ", ";</div></div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Target::getNamedOperandIdx(Target::ADD, Target::OpName::DST) => 0<br>Target::getNamedOperandIdx(Target::ADD, Target::OpName::SRC0) => 1<br>Target::getNamedOperandIdx(Target::ADD, Target::OpName::SRC1) => 2<br></blockquote>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The operand names are case insensitive, so $dst is equivalent to $DST.</blockquote>
</div><div><br></div><div style>In the second patch, it seems like all the values in the namespace Target::OpName are purely lowercase, such as `src0_sel` in:</div><div style><br></div><div style><div>- TII->getOperandIdx(Opcode, R600Operands::SRC0_SEL),</div>
<div>- TII->getOperandIdx(Opcode, R600Operands::SRC1_SEL),</div><div>- TII->getOperandIdx(Opcode, R600Operands::SRC2_SEL)</div><div>+ TII->getOperandIdx(Opcode, AMDGPU::OpName::src0_sel),</div><div>+ TII->getOperandIdx(Opcode, AMDGPU::OpName::src1_sel),</div>
<div>+ TII->getOperandIdx(Opcode, AMDGPU::OpName::src2_sel)</div><div><br></div><div style>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?).</div>
<div style><br></div><div style>Also, as a side note, the AMDGPU backend doesn't seem to have any content in <<a href="http://llvm.org/docs/CompilerWriterInfo.html">http://llvm.org/docs/CompilerWriterInfo.html</a>> (docs/CompilerWriterInfo.rst). Would you mind adding some links to the appropriate manuals?</div>
<div style><br></div><div style>Thanks,</div><div style><br></div><div style>-- Sean Silva</div></div>
</div><br></div></div>