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

[Tom Stellard]

On Mon, Jun 17, 2013 at 06:59:03PM -0700, Sean Silva wrote:
> 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.
> 

I will submit a new patch with all this documentation once Jakob
approves the approach taken in this patch.

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

The "patch versioning" is mainly to make it easier for myself and
testers to keep track of the different versions of the patch.  I deal
with a lot of patches and branches each day, and it's really easy to get
patch versions mixed up if they all have the same commit message.

> +
> +  // 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?).
>

Thanks for the feedback, I will work on making these changes.

> 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?
>

Sure, no problem.

-Tom