[llvm-commits] [llvm] r76177 - in /llvm/trunk: include/llvm/Target/TargetAsmInfo.h lib/Target/TargetAsmInfo.cpp utils/TableGen/AsmWriterEmitter.cpp

[David Greene]

On Friday 17 July 2009 16:39, Chris Lattner wrote:

> > +    /// InGroup - Determine whether we are in the middle of an
> > +    /// operand group.
> > +    bool InGroup() const { return GroupLevel != -1; }
>
> I don't really follow what grouping has to do with this stuff, maybe
> it is part of my confusion below.  Can you give an example of what
> you're trying to accomplish here?

I'm trying to avoid padding reg, [reg, reg] into three columns.  It should
be two.

> >     AsmWriterInst(const CodeGenInstruction &CGI, unsigned Variant);
> >
> >     /// MatchesAllButOneOp - If this instruction is exactly
> > identical to the
> > @@ -89,6 +112,70 @@
> >     void AddLiteralString(const std::string &Str) {
> >       // If the last operand was already a literal text string,
> > append this to
> >       // it, otherwise add a new operand.
>
> It seems very strange to me to handle this in "AddLiteralString".
> Shouldn't this be handled in the callers?  For example, the caller
> already knows about tabs:
>
>            case '\t': AddLiteralString("\\t"); break;
>
>          } else if (AsmString[DollarPos+1] == 't') {
>            AddLiteralString("\\t");
>
> Can you directly handle them there, instead of making AddLiteralString
> substantially more complex with string slicing and recursion?

Probably.  I'll take a look.

> > @@ -103,6 +190,18 @@
> >   if (OperandType == isLiteralTextOperand)
> >     return "O << \"" + Str + "\"; ";
> >
> > +  if (OperandType == isLiteralStatementOperand) {
> > +    return Str;
> > +  }
> > +
> > +  if (OperandType == isLiteralStatementOperand) {
> > +    return Str;
> > +  }
> > +
> > +  if (OperandType == isLiteralStatementOperand) {
> > +    return Str;
> > +  }
>
> Surely you only need to check this once?? :)

Yikes!

> > @@ -681,8 +784,16 @@
> >
> >   O << "  // Emit the opcode for the instruction.\n"
> >     << "  unsigned Bits = OpInfo[MI->getOpcode()];\n"
> > -    << "  if (Bits == 0) return false;\n"
> > -    << "  O << AsmStrs+(Bits & " << (1 << AsmStrBits)-1 << ");\n\n";
> > +    << "  if (Bits == 0) return false;\n\n";
> > +
> > +  O << "  std::string OpStr(AsmStrs+(Bits & " << (1 <<
> > AsmStrBits)-1 << "));\n"
>
> Again, the asmprinter has to be extremely fast because it is a
> bottleneck at -O0 compilation.  Creating temporary std::strings for
> asm opcodes is not acceptable.  I don't care how fast tblgen itself is
> (within reason :) but the generated code needs to be both very fast
> and ideally very small.  The reason we use the crazy "OpInfo" table
> and "AsmStrs" string is to make the code size of the generated
> asmprinter small.

Ok, I'll look at improving this.

> > +    << "  unsigned OperandColumn = 1;\n"
> > +    << "  O << OpStr;\n\n";
> > +
> > +  O << "  if (OpStr.find_last_of(\" \\t\") == OpStr.size()-1) {\n"
> > +    << "    O.PadToColumn(TAI->getOperandColumn(1));\n"
> > +    << "    OperandColumn = 2;\n"
> > +    << "  }\n\n";
>
> Likewise, why scan the string? Tblgen should know all of this about
> the instructions it is generating.  This seems like a hack.

Yes, I can probably fix this particular case.

                                -Dave