[llvm-commits] PATCH for PIC16 target.

[Dan Gohman]

On May 8, 2008, at 5:13 AM, Sanjiv.Gupta at microchip.com wrote:

> Please find attached files for Microchip's PIC16 backend.
> These files are to be placed under a new directory lib/Target/PIC16.
> Though the current PIC16 backend can handle few very elementary cases
> for code generation, these files enable basic llvm framework for PIC16
> target.
>
> The code builds on linux/mingw platforms without warnings/errors.
>
> I will send a separate patch for configure.

Ok, I've read through all the files. They look very straight-forward.  
I have a
number of minor comments below, but these files look fine to check in
whenever you're ready.

Along the way I saw a reference to A5.1.3. Is this in a document
that's publically available? Can you put a link to it somewhere?

In PIC16RegisterInfo.cpp:

 > // This file contains the PIC16 implementation of the MRegisterInfo  
class.

The code is right, but the comment still says MRegisterInfo. It should
say TargetRegisterInfo.

In PIC16TargetMachine.h:

 > #include "llvm/Target/TargetMachine.h"
 > #include "llvm/Target/TargetData.h"
 > #include "PIC16InstrInfo.h"
 > #include "PIC16Subtarget.h"
 > #include "PIC16ISelLowering.h"
 > #include "llvm/Target/TargetFrameInfo.h"

Please sort the include-files by subdirectory.

 > namespace llvm {
 >
 > /// PIC16TargetMachine
 > ///
 > class PIC16TargetMachine : public LLVMTargetMachine {
 >     PIC16Subtarget        Subtarget;
 > [...]
 >   };
 > } // end namespace llvm

Watch out for spacing.

In PIC16AsmPrinter.cpp:

 >   if (MO1.isRegister())
 >   {
 >      if(strcmp(TM.getRegisterInfo()->get(MO1.getReg()).Name,  
"SP")==0)
 >      {

LLVM style has the opening brace on the same line as the if.

In PIC16InstrInfo.h:

 >   PIC16InstrInfo(PIC16TargetMachine &TM);

This misses an explicit keyword.

In PIC16RegisterInfo.h:

 >   PIC16RegisterInfo(const TargetInstrInfo &tii);

This misses an explicit keyword.

In PIC16TargetAsmInfo.h:

 >     PIC16TargetAsmInfo(const PIC16TargetMachine &TM);

This misses an explicit keyword.

Dan