[llvm-commits] [llvm] r126538 - in /llvm/trunk/utils/TableGen: AsmWriterEmitter.cpp AsmWriterEmitter.h

[Chris Lattner]

On Feb 25, 2011, at 7:09 PM, Bill Wendling wrote:

> Author: void
> Date: Fri Feb 25 21:09:12 2011
> New Revision: 126538
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=126538&view=rev
> Log:
> A new TableGen feature! (Not turned on just yet.)
> 
>   InstAlias<{alias}, {aliasee}>;

Hi Bill,

Thanks for working on this, this is really cool functionality.  Since this went in with several patches that didn't review in flight, here's your long overdue review on the whole thing.

Lets talk about the generated code:

bool X86ATTInstPrinter::printAliasInstr(const MCInst *MI, raw_ostream &OS) {
  StringRef AsmString;
  std::map<StringRef, unsigned> OpMap;

Generating a std::map like this is unfortunate, slow and odd.  Why not generate something like:

SmallVector<std::pair<StringRef, unsigned>, 4>

.. and do a linear search?  Instructions don't have many operands, so your linear search is very likely to be faster than a single std::map allocation anyway.


You're generating some aliases like this, which aren't any simpler or better:

  case X86::SUB_FST0r:
...
    if (MI->getNumOperands() == 1 &&
        MI->getOperand(0).getReg() == X86::ST0) {
      // (SUB_FST0r ST0)
      AsmString = "fsub %st(0), %st(0)";
      break;
    }

Is there a way to filter these out to avoid bloating the generated alias printer?


  case X86::MOVSD:
    if (MI->getNumOperands() == 0) {
      // (MOVSD)
      AsmString = "movsd";
      break;
    }
    return false;

Why are you checking the # operands here, this can never have a non-zero number of operands?  Also, this is another example of an alias that isn't worth it.  This is giving us "movsd" instead of "movsl" which isn't cheaper or better in terms of understandability.  This is bloating out the alias table.  I think you need a cost model or something to decide if an alias is better than the aliasee.

  case X86::IDIV32r:
    if (MI->getNumOperands() == 1 &&
        MI->getOperand(0).isReg() &&
        regIsInRegisterClass(RC_GR32, MI->getOperand(0).getReg())) {
      // (IDIV32r GR32:$src)
      AsmString = "idivl $src, %eax";
      OpMap["src"] = 0;
      break;
    }
    return false;

This is another example of a case where the alias is longer than the aliased instruction.  A cost model should trim these out.



In Revision 127986:
+++ llvm/trunk/include/llvm/Target/TargetRegistry.h Sun Mar 20 23:13:46 2011
@@ -78,6 +78,7 @@
                                                TargetMachine &TM);
    typedef MCDisassembler *(*MCDisassemblerCtorTy)(const Target &T);
    typedef MCInstPrinter *(*MCInstPrinterCtorTy)(const Target &T,
+                                                  TargetMachine &TM,
                                                  unsigned SyntaxVariant,
                                                  const MCAsmInfo &MAI);
    typedef MCCodeEmitter *(*CodeEmitterCtorTy)(const Target &T,
@@ -286,11 +287,12 @@
      return MCDisassemblerCtorFn(*this);
    }

-    MCInstPrinter *createMCInstPrinter(unsigned SyntaxVariant,
+    MCInstPrinter *createMCInstPrinter(TargetMachine &TM,
+                                       unsigned SyntaxVariant,
                                       const MCAsmInfo &MAI) const {

This is *really really* unfortunate, because you've just completely broken the layering we had with the MC stuff not depending (much) on the TargetMachine.  This makes it impossible to use the instprinter without linking in all of the target libraries.  We must have a better fix for this.

I get the feeling that you knew that this was the wrong thing to do based on your FIXMEs.  You should have fixed the layering problem before tackling this.  I filed rdar://9482874 to track this internally.


I really love this functionality, but there still seems to be these two big issues to work out (cost model for aliases + layering violation).

-Chris