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

[Chris Lattner]

On Jul 17, 2009, at 7:25 AM, David Greene wrote:
> Author: greened
> Date: Fri Jul 17 09:24:46 2009
> New Revision: 76177
>
> Add logic to align instruction operands to columns for pretty- 
> printing.
> No target uses this currently.  This patch only adds the mechanism so
> that local installations can choose to enable this.

Ok.

> +++ llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp Fri Jul 17  
> 09:24:46 2009
> @@ -19,6 +19,7 @@
> #include "llvm/Support/Debug.h"
> #include "llvm/Support/MathExtras.h"
> #include <algorithm>
> +#include <sstream>
> #include <iostream>
> using namespace llvm;
>
> @@ -32,7 +33,11 @@
> // This should be an anon namespace, this works around a GCC warning.
> namespace llvm {
>   struct AsmWriterOperand {
> -    enum { isLiteralTextOperand, isMachineInstrOperand } OperandType;
> +    enum OpType {
> +      isLiteralTextOperand,
> +      isMachineInstrOperand,
> +      isLiteralStatementOperand
> +    } OperandType;

What is "isLiteralStatementOperand"?  While the state of documentation  
wasn't great before you go here, it would be nice to add a comment  
about what this is.

> @@ -78,6 +85,22 @@
>     std::vector<AsmWriterOperand> Operands;
>     const CodeGenInstruction *CGI;
>
> +    /// MAX_GROUP_NESTING_LEVEL - The maximum number of group nesting
> +    /// levels we ever expect to see in an asm operand.
> +    static const int MAX_GROUP_NESTING_LEVEL = 10;
> +
> +    /// GroupLevel - The level of nesting of the current operand
> +    /// group, such as [reg + (reg + offset)].  -1 means we are not  
> in
> +    /// a group.
> +    int GroupLevel;
> +
> +    /// GroupDelim - Remember the delimeter for a group operand.
> +    char GroupDelim[MAX_GROUP_NESTING_LEVEL];
> +
> +    /// 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?


> +
>     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?
>
> @@ -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?? :)

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

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

-Chris