[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

[David A. Greene]

On Monday 03 August 2009 01:46, Chris Lattner wrote:

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

Agreed that getOperandColumn could be moved.  But the column and operand
length information really is target-specific.

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

Interesting idea.

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

I agree that this could be pretty good.  Unfortunately, I don't have the
time to implement it at the moment.  It would involve a bit of TableGen
hacking, I imagine.  I might be able to get to it after I've got my current
set of patches sent upstream (well over a hundred at the moment).

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

Yes, the check could be removed then.  Right now the check fails if
FirstOperandColumn == 0 or MaxOperandLength == 0.

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


Oh!  This changed since the last time I looked at it.  It used to be that
the tabs were embedded into the AsmStrs array and some opcodes had tabs after 
them and others didn't.  I see now that in X86GenAsmWriter1.inc the asm 
boilerplate emits the tab after the opcode:

  O << "\t";

  // Emit the opcode for the instruction.
  unsigned Bits = OpInfo[MI->getOpcode()];
  if (Bits == 0) return false;

How interesting.  But then how does this work?

  case 10:
    // CMPPDrmi, CMPPDrri
    O << "pd"; 
    PadToColumn(OperandColumn++);

    printOperand(MI, 0); 
    O << ", "; 
    break;

Looks like the "pd" for packed cmp is emitted after the tab!

What am I missing?

                           -Dave