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

[Bill Wendling]

On May 21, 2011, at 10:29 PM, Chris Lattner wrote:

> 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.
> 
Okay. That shouldn't be too bad.

> 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?
> 
Yes. I added that after this patch. Dan pointed out that some of the aliases are not as nice as what they're aliasing. We can disable them on a case-by-case basis by adding '0' to the end of the InstAlias. (Same for the MOVSD you mentioned below.)

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

It's probably a bug in how this code is generated that it doesn't recognize a zero number of operands as being impossible.

> 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 my opinion, it's not necessarily the length that's a factor, but which form of the instruction gives the best information. I'm hoping that the "opt-out" works sufficiently. If it doesn't, then I can try to come up with a heuristic that can.

> 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.
> 
Yes, I knew it was wrong, but it's done this way in the AsmPrinter and for similar reasons (and with FIXMEs to remove it when we're able to). This is definitely a temporary situation. Once we can get the information from the Triple, this can be removed.

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

-bw