[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 Aug 1, 2009, at 8:01 AM, David Greene wrote:
Chris Lattner wrote:
>> 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?
>
> Two things:
>
> - There aren't any tabs in X86InstInfo.td (after the mnemonic, that  
> is)

Aha, duh.  Ok :).

> - FirstOperandColumn = 0 and MaxOperandLength = 0 in X86TargetAsmInfo
>   which disables the padding.

Ok, this gets back to another question I have.  I don't think that  
these should be properties of TAI, at least not unless there are other  
clients of this information.  The TAI::getOperandColumn() method in  
particular seems like an implementation detail of the asmprinter, not  
something that should be in TAI.

However, more broadly, these are not properties that should be checked  
when the asmprinter is running.  If these were properties of the  
"InstrInfo" class in the .td files, then X86 (for example), could be  
defined like this (X86.td):

def X86InstrInfo : InstrInfo {
   let FirstOperandColumn = 42;  // or whatever.
   let MaxOperandLength = 20;
...

The nice thing about doing this is that if the target doesn't opt in  
to this, that the tblgen generated code for the asmwriter *wouldn't  
treat tab specially at all* and would not have to dynamically decide  
not to do column padding.  The generated code would still have to  
dynamically check whether verbose-asm is enabled, but there should be  
no need to dynamically check to see if the asm syntax likes tabs.   
Making these values constants also allows them to constant fold in the  
generated code.

>>> +/// 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" ?
>
> No, it's testing 0 for any operand, not just the first.

Ok, maybe I'm not following it right then.  When does this check  
fail?  Can this be removed by moving the "column padding turned on"  
flag to the .td files?

>>> @@ -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?
>
> Because AsmStrs gets generated by TableGen.  I'm not sure that .td
> files actually have control over it since TableGen does the encoding
> compaction itself.  Look at X86GenAsdmWriter[1].inc for example.

I'm not following what you mean here.  Can you give me an example that  
shows what this does?

-Chris