[llvm-commits] [llvm] r77740 - in /llvm/trunk: include/llvm/CodeGen/AsmPrinter.h lib/CodeGen/AsmPrinter/AsmPrinter.cpp test/CodeGen/X86/2009-04-17-tls-fast.ll test/CodeGen/X86/tls1-pic.ll test/CodeGen/X86/tls2-pic.ll test/CodeGen/X86/tls3-pic.ll test/CodeGen/X86/tls4-pic.ll utils/TableGen/AsmWriterEmitter.cpp

[Chris Lattner]

On Jul 31, 2009, at 2:57 PM, David Greene wrote:
Author: greened
> Date: Fri Jul 31 16:57:10 2009
> New Revision: 77740
>
> URL: http://llvm.org/viewvc/llvm-project?rev=77740&view=rev
> Log:
>
> Simplify operand padding by keying off tabs in the asm stream.  If
> padding is disabled, tabs get replaced by spaces except in the case of
> the first operand, where the tab is output to line up the operands  
> after
> the mnemonics.

Wow, this is awesome!

I'm not seeing this kick in.  On X86, I'm seeing stuff like:

_get_results:
LBB2_0: ## entry
         pushl   %ebx
         pushl   %edi
         pushl   %esi
         subl    $48, %esp
         testl   %edx, %edx
         movl    %ecx, %esi
         je      LBB2_4  ## bb


Shouldn't the esp/edx/esi be lined up?  What am I missing here?

> +/// PadToColumn - This gets called every time a tab is emitted.  If
> +/// column padding is turned on, we replace the tab with the
> +/// appropriate amount of padding.  If not, we replace the tab with a
> +/// space, except for the first operand so that initial operands are
> +/// always lined up by tabs.
> +void AsmPrinter::PadToColumn(unsigned Operand) const {
> +  if (TAI->getOperandColumn(Operand) > 0) {

Is this just a complex way to test for "FirstOperandColumn != 0" ?

> +    O.PadToColumn(TAI->getOperandColumn(Operand), 1);
> +  }
> +  else {
> +    if (Operand == 1) {

How about "else if"?

> +      // Emit the tab after the mnemonic.
> +      O << '\t';

This comment is borderline self evident.  Can you reword it something  
like: "If this is the first operand after the mnemonic, we actually do  
tab it out to make the operands line up.  For others, we just insert  
space for simplicity."

> @@ -852,6 +733,8 @@
>     << "  if (TAI->getOperandColumn(1) > 0) {\n"
>     << "    // Don't emit trailing whitespace, let the column  
> padding do it.  This\n"
>     << "    // guarantees that a stray long opcode + tab won't upset  
> the alignment.\n"
> +    << "    // We need to handle this special case here because  
> sometimes the initial\n"
> +    << "    // mnemonic string includes a tab or space and  
> sometimes it doesn't.\n"
>     << "    unsigned OpLength = std::strlen(AsmStrs+(Bits & " << (1  
> << AsmStrBits)-1 << "));\n"
>     << "    if (OpLength > 0 &&\n"
>     << "        ((AsmStrs+(Bits & " << (1 << AsmStrBits)-1 << ")) 
> [OpLength-1] == ' ' ||\n"

Should this just be a considered a bug in .td files that don't do this  
right?  What is the harm of just leaving this out?

-Chris