[LLVMdev] X86AsmPrinter + MASM and NASM backends

[Chris Lattner]

On Thu, 30 Jun 2005, Aaron Gray wrote:
> I have "refactored" the X86AsmPrinter into a number of files ready for 
> the MASM and NASM backends to be added.

Nice!

> There is a new namespace llvm::X86 to replace the anomonous namespace as 
> this does not work accross mutiple .h and .cpp files. Other than that 
> everything is pritty straight forward, t may possibly need tweeking 
> though.

Ok, here are my requests.  Overall the code looks really good.  I would 
appreciate it if you could make the following changes:

1. At the top of the X86AsmPrinter.cpp file, you note that the code was
    refactored.  The comments should indicate the current state of the
    code, not the history.  Please just say that it supports the X/Y/Z
    subclasses, not that it was changed to support them.  Likewise in any
    other similar comments.  In the CVS commit message, we will explain the
    history of the code.
2. In the .cpp files, please use the 'using namespace llvm;' idiom to
    avoid placing the entire C++ file in two sets of namespaces (which
    indents everything).  Check out other .cpp files for how they are
    written as examples.
3. Please #include X86ASmPrinter.h first in X86asmPrinter.cpp (see the
    coding standard for justification).  This will expose the fact that the
    .h file needs some #includes to be self contained.
4. I think it would be a good idea to make the isScale/isMem methods
    inline in the header file, even if that means more #includes are
    needed in the .h file.  These are pretty performance sensitive for the
    printers, so it would be good for them to be inlined.
5. Please add #include guards around the headers (#ifndef XX_H/#define
    XX_H), like the other llvm headers.

Once you've made these high level changes, please send the code out again 
and I'll check it once more.  Again, the high-level refactoring looks 
great, and I'm glad you seperated the refactoring from the addition of
new functionality!

> It has been built under MS VS2003, but I am not sure how to add it to 
> the makefiles for the Cygwin and Linux platforms, help on this would be 
> appreciated.

There should be no changes required for the Makefiles, unlike those silly 
VC project files ;-P

> Afaik there are no specific tests for the X86AsmPrinter. If not it would 
> be good to create some.

Correct.  X86AsmPrinter is currently only used for debugging: no targets 
use it.  I suspect your target will be the first real test!  This also 
means that if it is doing something wrong, you can change it!

> Also I am wondering about how to go about creating tests for the MASM 
> and NASM backends, hints and help are welcomed.

I think the best way to do this is to start pumping code through it.  If 
you can get the llvmgcc front-end to work in your environment, llvm-test 
is the place to start.

Thanks for the great enhancement!

-Chris

-- 
http://nondot.org/sabre/
http://llvm.cs.uiuc.edu/