Wed May 14 01:07:55 PDT 2008

Hi Sanjiv,

On May 13, 2008, at 2:02 AM, Sanjiv Gupta wrote:

> Adding files for Microchip's PIC16 target.
>
Excellent work! Thank you. I just have a few stylistic changes.

Overall comments:

- Please remove all tabs from the files (except for Makefile, of  
course).
- Please verify that all code fits in 80-columns.

> A brief description about PIC16:
> ===============================
> PIC16 is an 8-bit microcontroller with only one 8-bit register which  
> is the
> accumulator. All arithmetic/load/store operations are 8-bit only.
> The architecture has two address spaces: program and data. The  
> program memory
> is divided into 2K pages and the data memory is divided into banks  
> of 128 byte, with only 80 usable bytes, resulting in an non- 
> contiguous data memory.
>
> It supports direct data memory access (by specifying the address as  
> part of the instruction) and indirect data and program memory access  
> (in an unorthodox fashion which utilize a 16 bit pointer register).
>
> Two classes of registers exist: (8-bit class which is only one
> accumulator) (16-bit class, which contains one or more 16 bit
> pointer(s))
>

Would you be interested in writing this information into the  
documentation? Perhaps here: docs/CodeGenerator.html

You know, in your copious spare time. ;-)

> Added: llvm/trunk/lib/Target/PIC16/PIC16.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PIC16/PIC16.td?rev=51027&view=auto
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Target/PIC16/PIC16.td (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16.td Tue May 13 04:02:57 2008
> @@ -0,0 +1,46 @@
> +//===- PIC16.td - Describe the PIC16 Target Machine -----------*-  
> tblgen -*-==//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +// This is the top level entry point for the PIC16 target.
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +// Target-independent interfaces
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +include "../Target.td"
> +
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +// Descriptions
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +include "PIC16RegisterInfo.td"
> +include "PIC16CallingConv.td"
> +include "PIC16InstrInfo.td"
> +
> +def PIC16InstrInfo : InstrInfo {
> +  let TSFlagsFields = [];
> +  let TSFlagsShifts = [];
> +}
> +
> +
> +
> +// Not currently supported, but work as SubtargetFeature placeholder.

Do you want to add this as a FIXME or a note in a PIC16-specific  
README file?

> +def FeaturePIC16Old : SubtargetFeature<"pic16old", "IsPIC16Old",  
> "true",
> +                                      "PIC16 Old ISA Support">;
> +

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Target/PIC16/PIC16AsmPrinter.cpp (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16AsmPrinter.cpp Tue May 13  
> 04:02:57 2008
> @@ -0,0 +1,569 @@
> +//===-- PIC16AsmPrinter.cpp - PIC16 LLVM assembly writer  
> ------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//
> +// This file contains a printer that converts from our internal  
> representation
> +// of machine-dependent LLVM code to PIC16 assembly language.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#define DEBUG_TYPE "asm-printer"
> +#include "PIC16.h"
> +#include "PIC16TargetMachine.h"
> +#include "PIC16ConstantPoolValue.h"
> +#include "PIC16InstrInfo.h"
> +#include "llvm/Constants.h"
> +#include "llvm/DerivedTypes.h"
> +#include "llvm/Module.h"
> +#include "llvm/ADT/SetVector.h"
> +#include "llvm/ADT/Statistic.h"
> +#include "llvm/ADT/StringExtras.h"
> +#include "llvm/CodeGen/AsmPrinter.h"
> +#include "llvm/CodeGen/MachineFunctionPass.h"
> +#include "llvm/CodeGen/MachineConstantPool.h"
> +#include "llvm/CodeGen/MachineFrameInfo.h"
> +#include "llvm/CodeGen/MachineInstr.h"
> +#include "llvm/Support/CommandLine.h"
> +#include "llvm/Support/Debug.h"
> +#include "llvm/Support/Mangler.h"
> +#include "llvm/Support/MathExtras.h"
> +#include "llvm/Target/TargetAsmInfo.h"
> +#include "llvm/Target/TargetData.h"
> +#include "llvm/Target/TargetMachine.h"
> +#include "llvm/Target/TargetOptions.h"
> +#include <cctype>
> +
> +using namespace llvm;
> +
> +STATISTIC(EmittedInsts, "Number of machine instrs printed");
> +
> +namespace {
> +  struct VISIBILITY_HIDDEN PIC16AsmPrinter : public AsmPrinter {
> +    PIC16AsmPrinter(std::ostream &O, TargetMachine &TM, const  
> TargetAsmInfo *T)
> +      : AsmPrinter(O, TM, T) {
> +    }
> +
> +
> +    /// We name each basic block in a Function with a unique  
> number, so
> +    /// that we can consistently refer to them later. This is cleared
> +    /// at the beginning of each call to runOnMachineFunction().
> +    ///
> +    typedef std::map<const Value *, unsigned> ValueMapTy;
> +    ValueMapTy NumberForBB;
> +
> +    /// Keeps the set of GlobalValues that require non-lazy- 
> pointers for
> +    /// indirect access.
> +    std::set<std::string> GVNonLazyPtrs;
> +
> +    /// Keeps the set of external function GlobalAddresses that the  
> asm
> +    /// printer should generate stubs for.
> +    std::set<std::string> FnStubs;
> +
> +    /// True if asm printer is printing a series of CONSTPOOL_ENTRY.
> +    bool InCPMode;
> +
> +    virtual const char *getPassName() const {
> +      return "PIC16 Assembly Printer";
> +    }
> +
> +    void printOperand(const MachineInstr *MI, int opNum,
> +                      const char *Modifier = 0);
> +
> +    void printSOImmOperand(const MachineInstr *MI, int opNum);
> +
> +    void printAddrModeOperand(const MachineInstr *MI, int OpNo);
> +
> +    void printRegisterList(const MachineInstr *MI, int opNum);
> +    void printCPInstOperand(const MachineInstr *MI, int opNum,
> +                            const char *Modifier);
> +
> +
> +    bool printInstruction(const MachineInstr *MI);  // autogenerated.
> +    void emitFunctionStart(MachineFunction &F);
> +    bool runOnMachineFunction(MachineFunction &F);
> +    bool doInitialization(Module &M);
> +    bool doFinalization(Module &M);
> +
> +    virtual void  
> EmitMachineConstantPoolValue(MachineConstantPoolValue *MCPV);
> +
> +    void getAnalysisUsage(AnalysisUsage &AU) const;
> +
> +    public:

Please re-indent "public:" and other access-specifier.

> +    void SwitchToTextSection(const char *NewSection,
> +			     const GlobalValue *GV = NULL);
> +    void SwitchToDataSection(const char *NewSection,
> +			     const GlobalValue *GV = NULL);
> +    void SwitchToDataOvrSection(const char *NewSection,
> +				const GlobalValue *GV = NULL);
> +  };
> +} // end of anonymous namespace
> +
> +#include "PIC16GenAsmWriter.inc"
> +
> +/// createPIC16CodePrinterPass - Returns a pass that prints the PIC16
> +/// assembly code for a MachineFunction to the given output stream,
> +/// using the given target machine description.  This should work
> +/// regardless of whether the function is in SSA form.
> +///
> +FunctionPass *llvm::createPIC16CodePrinterPass(std::ostream &o,
> +                                               PIC16TargetMachine  
> &tm) {
> +  return new PIC16AsmPrinter(o, tm, tm.getTargetAsmInfo());
> +}
> +
> +void PIC16AsmPrinter::getAnalysisUsage(AnalysisUsage &AU) const
> +{
> +  // Currently unimplemented.
> +}
> +
> +
> +void PIC16AsmPrinter ::

Extra space before ::.
>
> +EmitMachineConstantPoolValue(MachineConstantPoolValue *MCPV)
> +{
> +  printDataDirective(MCPV->getType());
> +
> +  PIC16ConstantPoolValue *ACPV = (PIC16ConstantPoolValue*)MCPV;
> +  GlobalValue *GV = ACPV->getGV();
> +  std::string Name = GV ? Mang->getValueName(GV) : TAI- 
> >getGlobalPrefix();
> +  if (!GV)
> +    Name += ACPV->getSymbol();
> +  if (ACPV->isNonLazyPointer()) {
> +    GVNonLazyPtrs.insert(Name);
> +    O << TAI->getPrivateGlobalPrefix() << Name << "$non_lazy_ptr";
> +  } else if (ACPV->isStub()) {
> +    FnStubs.insert(Name);
> +    O << TAI->getPrivateGlobalPrefix() << Name << "$stub";
> +  } else
> +    O << Name;

The formatting after the "} else" line is confusing. Please place  
braces around the statement in the "else" case here and reformat the  
rest of the code accordingly.

> +    if (ACPV->hasModifier()) O << "(" << ACPV->getModifier() << ")";
> +
> +    if (ACPV->getPCAdjustment() != 0) {
> +      O << "-(" << TAI->getPrivateGlobalPrefix() << "PC"
> +        << utostr(ACPV->getLabelId())
> +        << "+" << (unsigned)ACPV->getPCAdjustment();
> +
> +      if (ACPV->mustAddCurrentAddress())
> +        O << "-.";
> +
> +      O << ")";
> +    }
> +    O << "\n";
> +
> +    // If the constant pool value is a extern weak symbol, remember  
> to emit
> +    // the weak reference.
> +    if (GV && GV->hasExternalWeakLinkage())
> +      ExtWeakSymbols.insert(GV);
> +}
> +
> +/// Emit the directives used by ASM on the start of functions

Please place the name of the method in the comment.

> +void PIC16AsmPrinter:: emitFunctionStart(MachineFunction &MF)

Extra space after the ::.

> +{
> +   // Print out the label for the function.
> +   const Function *F = MF.getFunction();
> +   MachineFrameInfo *FrameInfo = MF.getFrameInfo();
> +   if (FrameInfo->hasStackObjects()) {	
> +     int indexBegin = FrameInfo->getObjectIndexBegin();
> +     int indexEnd = FrameInfo->getObjectIndexEnd();
> +     while (indexBegin<indexEnd) {
> +       if (indexBegin ==0)
> +         SwitchToDataOvrSection(F->getParent()- 
> >getModuleIdentifier().c_str(),
> +			 	F);
> +		
> +         O << "\t\t" << CurrentFnName << "_" << indexBegin << " "  
> << "RES"
> +	   << " " << FrameInfo->getObjectSize(indexBegin) << "\n" ;
> +         indexBegin++;
> +     }
> +   }
> +   SwitchToTextSection(CurrentFnName.c_str(), F);
> +   O << "_" << CurrentFnName << ":" ;
> +   O << "\n";
> +}
> +
> +
> +/// runOnMachineFunction - This uses the printInstruction()
> +/// method to print assembly for each instruction.
> +///
> +bool PIC16AsmPrinter::
> +runOnMachineFunction(MachineFunction &MF)
> +{
> +
> +  // DW.SetModuleInfo(&getAnalysis<MachineModuleInfo>());
> +  SetupMachineFunction(MF);
> +  O << "\n";
> +
> +  // NOTE: we don't print out constant pools here, they are handled  
> as
> +  // instructions.
> +  O << "\n";
> +
Why two newlines here? I don't think we use "NOTE" in other places. If  
you just comment the code without it, it won't look like some sort of  
TODO or FIXME (like I just mistook it for :-).

> +  // What's my mangled name?
> +  CurrentFnName = Mang->getValueName(MF.getFunction());
> +
> +  // Emit the function start directives
> +  emitFunctionStart(MF);
> +
> +  // Emit pre-function debug information.
> +  // DW.BeginFunction(&MF);
> +
Please remove dead code.

> +  // Print out code for the function.
> +  for (MachineFunction::const_iterator I = MF.begin(), E = MF.end();
> +       I != E; ++I) {
> +    // Print a label for the basic block.
> +    if (I != MF.begin()) {
> +      printBasicBlockLabel(I, true);
> +      O << '\n';
> +    }
> +    for (MachineBasicBlock::const_iterator II = I->begin(), E = I- 
> >end();
> +         II != E; ++II) {
> +      // Print the assembly for the instruction.
> +      O << '\t';
> +      printInstruction(II);
> +      ++EmittedInsts;
> +    }
> +  }
> +
> +  // Emit post-function debug information.
> +  // DW.EndFunction();
> +
Dead.

> +  // We didn't modify anything.
> +  return false;
> +}
> +
> +void PIC16AsmPrinter::
> +printOperand(const MachineInstr *MI, int opNum, const char *Modifier)
> +{
> +  const MachineOperand &MO = MI->getOperand(opNum);
> +  const TargetRegisterInfo  &RI = *TM.getRegisterInfo();
> +
> +  switch (MO.getType())
> +  {

Could you put the brace on the same line as the "switch" and realign  
all of the case statements?

> +    case MachineOperand::MO_Register:
> +    {

The case statements don't need braces.

>
> +      if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()))
> +        O << RI.get(MO.getReg()).Name;
> +      else
> +        assert(0 && "not implemented");
> +      break;
> +    }
> +    case MachineOperand::MO_Immediate:
> +    {
> +      if (!Modifier || strcmp(Modifier, "no_hash") != 0)
> +        O << "#";
> +      O << (int)MO.getImm();
> +      break;
> +    }
> +    case MachineOperand::MO_MachineBasicBlock:
> +    {
> +      printBasicBlockLabel(MO.getMBB());
> +      return;
> +    }
> +    case MachineOperand::MO_GlobalAddress:
> +    {
> +      O << Mang->getValueName(MO.getGlobal())<<'+'<<MO.getOffset();
> +      break;
> +    }
> +    case MachineOperand::MO_ExternalSymbol:
> +    {
> +      O << MO.getSymbolName();
> +      break;
> +    }
> +    case MachineOperand::MO_ConstantPoolIndex:
> +    {
> +      O << TAI->getPrivateGlobalPrefix() << "CPI" <<  
> getFunctionNumber()
> +        << '_' << MO.getIndex();
> +      break;
> +    }
> +    case MachineOperand::MO_FrameIndex:
> +    {
> +      O << "_" << CurrentFnName
> +        << '+' << MO.getIndex();
> +      break;
> +    }
> +    case MachineOperand::MO_JumpTableIndex:
> +    {
> +      O << TAI->getPrivateGlobalPrefix() << "JTI" <<  
> getFunctionNumber()
> +        << '_' << MO.getIndex();
> +      break;
> +    }
> +    default:
> +    {
> +      O << "<unknown operand type>"; abort ();
> +      break;
> +    }
> +  } // end switch.
> +}
> +
> +static void
> +printSOImm(std::ostream &O, int64_t V, const TargetAsmInfo *TAI)
> +{
> +  assert(V < (1 << 12) && "Not a valid so_imm value!");
> +  unsigned Imm = V;
> +
Why do you use a temporary variable? If you want it as an unsigned,  
you can cast it in the "O<<Imm" statement.

> +  O << Imm;
> +}
> +
> +/// printSOImmOperand - SOImm is 4-bit rotate amount in bits 8-11  
> with 8-bit
> +/// immediate in bits 0-7.

s/rotate/rotated/

> +void PIC16AsmPrinter::
> +printSOImmOperand(const MachineInstr *MI, int OpNum)
> +{
> +  const MachineOperand &MO = MI->getOperand(OpNum);
> +  assert(MO.isImmediate() && "Not a valid so_imm value!");
> +  printSOImm(O, MO.getImm(), TAI);
> +}
> +
> +
> +void PIC16AsmPrinter:: printAddrModeOperand(const MachineInstr *MI,  
> int Op)
> +{
> +  const MachineOperand &MO1 = MI->getOperand(Op);
> +  const MachineOperand &MO2 = MI->getOperand(Op+1);
> +
> +  if (MO2.isFrameIndex ()) {
> +    printOperand(MI, Op+1);
> +    return;
> +  }
> +
> +  if (!MO1.isRegister()) {   // FIXME: This is for CP entries, but  
> isn't right.
> +    printOperand(MI, Op);
> +    return;
> +  }
> +
> +  // If this is Stack Slot
> +  if (MO1.isRegister()) {
> +    if(strcmp(TM.getRegisterInfo()->get(MO1.getReg()).Name, "SP")==0)
> +    {
Please put brace on same line as "if"

>
> +      O << CurrentFnName <<"_"<< MO2.getImm();
> +      return;
> +    }
> +    O << "[" << TM.getRegisterInfo()->get(MO1.getReg()).Name;
> +    O << "+";
> +    O << MO2.getImm();
> +    O << "]";
> +    return;
> +  }
> +
> +  O << "[" << TM.getRegisterInfo()->get(MO1.getReg()).Name;
> +  O << "]";
> +}
> +
> +
> +void PIC16AsmPrinter:: printRegisterList(const MachineInstr *MI,  
> int opNum)

Extra space after ::.

> +{
> +  O << "{";
> +  for (unsigned i = opNum, e = MI->getNumOperands(); i != e; ++i) {
> +    printOperand(MI, i);
> +    if (i != e-1) O << ", ";
> +  }
> +  O << "}";
> +}
> +
...
>
> +bool PIC16AsmPrinter:: doInitialization(Module &M)
>
> +{
> +  // Emit initial debug information.
> +  // DW.BeginModule(&M);
> +
Dead code.

>
> +  bool Result = AsmPrinter::doInitialization(M);
> +  return Result;
> +}
> +
> +bool PIC16AsmPrinter:: doFinalization(Module &M)
> +{
> +  const TargetData *TD = TM.getTargetData();
> +
> +  for (Module::const_global_iterator I = M.global_begin(), E =  
> M.global_end();
> +       I != E; ++I) {
> +    if (!I->hasInitializer())   // External global require no code
> +      continue;
> +
> +    if (EmitSpecialLLVMGlobal(I)) {
> +      continue;
> +    }
> +
> +    std::string name = Mang->getValueName(I);
> +    Constant *C = I->getInitializer();
> +    const Type *Type = C->getType();

Please don't name the variable "Type". It's confusing for those  
reading. Please pick either "Ty" or another more descriptive name.

> +    unsigned Size = TD->getABITypeSize(Type);
> +    unsigned Align = TD->getPreferredAlignmentLog(I);
> +
> +    const char *VisibilityDirective = NULL;
> +    if (I->hasHiddenVisibility())
> +      VisibilityDirective = TAI->getHiddenDirective();
> +    else if (I->hasProtectedVisibility())
> +      VisibilityDirective = TAI->getProtectedDirective();
> +
> +    if (VisibilityDirective)
> +      O << VisibilityDirective << name << "\n";
> +
> +    if (C->isNullValue()) {
> +      if (I->hasExternalLinkage()) {
> +        if (const char *Directive = TAI->getZeroFillDirective()) {
> +          O << "\t.globl\t" << name << "\n";
> +          O << Directive << "__DATA__, __common, " << name << ", "
> +            << Size << ", " << Align << "\n";
> +          continue;
> +        }
> +      }
> +
> +      if (!I->hasSection() &&
> +          (I->hasInternalLinkage() || I->hasWeakLinkage() ||
> +           I->hasLinkOnceLinkage())) {
> +        if (Size == 0) Size = 1;   // .comm Foo, 0 is undefined,  
> avoid it.
> +        if (!NoZerosInBSS && TAI->getBSSSection())
> +           SwitchToDataSection(M.getModuleIdentifier().c_str(), I);
> +        else
> +          SwitchToDataSection(TAI->getDataSection(), I);
> +        if (TAI->getLCOMMDirective() != NULL) {
> +          if (I->hasInternalLinkage()) {
> +            O << TAI->getLCOMMDirective() << name << "," << Size;
> +          } else
> +            O << TAI->getCOMMDirective()  << name << "," << Size;
> +        } else {
> +          if (I->hasInternalLinkage())
> +             O << "\t.local\t" << name << "\n";
> +
> +          O << TAI->getCOMMDirective() <<"\t" << name << " "  
> <<"RES"<< " "
> +	    << Size;
> +          O << "\n\t\tGLOBAL" <<" "<< name;
> +          if (TAI->getCOMMDirectiveTakesAlignment())
> +             O << "," << (TAI->getAlignmentIsInBytes() ? (1 <<  
> Align) : Align);
> +        }
> +        continue;
> +      }
> +    }
> +
> +    switch (I->getLinkage())
> +    {
Brace.

>
> +    case GlobalValue::AppendingLinkage:
> +    {
Braces not needed.
>
> +      // FIXME: appending linkage variables should go into a  
> section of
> +      // their name or something.  For now, just emit them as  
> external.
> +      // Fall through
> +    }
> +    case GlobalValue::ExternalLinkage:
> +    {
> +      O << "\t.globl " << name << "\n";
> +      // FALL THROUGH
> +    }
...

> --- llvm/trunk/lib/Target/PIC16/PIC16CallingConv.td (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16CallingConv.td Tue May 13  
> 04:02:57 2008
> @@ -0,0 +1,17 @@
> +//===- PIC16CallingConv.td - Calling Conventions Sparc -----*-  
> tablegen -*-===//

s/Sparc/PIC16/

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Target/PIC16/PIC16ISelDAGToDAG.cpp (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16ISelDAGToDAG.cpp Tue May 13  
> 04:02:57 2008
> +class VISIBILITY_HIDDEN PIC16DAGToDAGISel : public SelectionDAGISel {
> +
> +  /// TM - Keep a reference to PIC16TargetMachine.
> +  PIC16TargetMachine &TM;
> +
> +  /// PIC16Lowering - This object fully describes how to lower LLVM  
> code to an
> +  /// PIC16-specific SelectionDAG.
> +  PIC16TargetLowering PIC16Lowering;
> +
> +  /// Subtarget - Keep a pointer to the PIC16Subtarget around so  
> that we can
> +  /// make the right decision when generating code for different  
> targets.
> +  //TODO: add initialization on constructor
> +  //const PIC16Subtarget *Subtarget;
> +
Please use "FIXME" instead of "TODO". If this is dead, please delete it.

> +private:
> +  // Include the pieces autogenerated from the target description.
> +  #include "PIC16GenDAGISel.inc"
> +
Please put in column 1.

>
> +  SDNode *Select(SDOperand N);
> +
> +  // Select addressing mode. currently assume base + offset addr  
> mode.
> +  bool SelectAM(SDOperand Op, SDOperand N, SDOperand &Base,  
> SDOperand &Offset);
> +  bool SelectDirectAM(SDOperand Op, SDOperand N, SDOperand &Base,
> +		      SDOperand &Offset);
> +  bool StoreInDirectAM(SDOperand Op, SDOperand N, SDOperand &fsr);
> +  bool LoadFSR(SDOperand Op, SDOperand N, SDOperand &Base,  
> SDOperand &Offset);
> +  bool LoadNothing(SDOperand Op, SDOperand N, SDOperand &Base,
> +		   SDOperand &Offset);
> +
> +  // getI8Imm - Return a target constant with the specified
> +  // value, of type i8.
> +  inline SDOperand getI8Imm(unsigned Imm) {
> +    return CurDAG->getTargetConstant(Imm, MVT::i8);
> +  }
> +
> +
> +  #ifndef NDEBUG
column 1
>
> +  unsigned Indent;
> +  #endif
ditto.
>
> +};
> +
> +}
> +
> +/// InstructionSelectBasicBlock - This callback is invoked by
> +/// SelectionDAGISel when it has created a SelectionDAG for us to  
> codegen.
> +void PIC16DAGToDAGISel::InstructionSelectBasicBlock(SelectionDAG &SD)
> +{
> +  DEBUG(BB->dump());
> +  // Codegen the basic block.
> +  #ifndef NDEBUG
> +  DOUT << "===== Instruction selection begins:\n";
> +  Indent = 0;
> +  #endif
> +
> +  // Select target instructions for the DAG.
> +  SD.setRoot(SelectRoot(SD.getRoot()));
> +
> +  #ifndef NDEBUG
> +  DOUT << "===== Instruction selection ends:\n";
> +  #endif
> +
It's not entirely necessary to put DOUT statements inside of NDEBUG  
#ifdefs. They are that way automatically.

>
> +bool PIC16DAGToDAGISel::
> +SelectDirectAM (SDOperand Op, SDOperand N, SDOperand &Base,  
> SDOperand &Offset)
> +{
> +  GlobalAddressSDNode *GA;
> +  ConstantSDNode      *GC;
> +
> +  // if Address is FI, get the TargetFrameIndex.
> +  if (FrameIndexSDNode *FIN = dyn_cast<FrameIndexSDNode>(N)) {
> +    cout << "--------- its frame Index\n";

cout? Or DOUT?

...

> +//don't thake this seriously, it will change
> +bool PIC16DAGToDAGISel::
> +LoadNothing (SDOperand Op, SDOperand N, SDOperand &Base, SDOperand  
> &Offset)
> +{
> +  GlobalAddressSDNode *GA;
> +  if (N.getOpcode() == ISD::GlobalAddress) {
> +    GA = dyn_cast<GlobalAddressSDNode>(N);
> +    cout << "==========" << GA->getOffset() << "\n";

cout or DOUT?

> +/// Select instructions not customized! Used for
> +/// expanded, promoted and normal instructions

Name of method in comments please. End sentence in a period.

> +SDNode* PIC16DAGToDAGISel::Select(SDOperand N)
> +{
> +  SDNode *Node = N.Val;
> +  unsigned Opcode = Node->getOpcode();
> +
> +  // Dump information about the Node being selected
> +  #ifndef NDEBUG

Line 1. etc. DOUT and DEBUG are already NDEBUG aware. See include/llvm/ 
Support/Debug.h for more information.

> +  DOUT << std::string(Indent, ' ') << "Selecting: ";
> +  DEBUG(Node->dump(CurDAG));
> +  DOUT << "\n";
> +  Indent += 2;
> +  #endif
> +

...
>
> +  ///
> +  // Instruction Selection not handled by custom or by the
> +  // auto-generated tablegen selection should be handled here.
> +  ///

This looks more like a FIXME. Please add one.

> +  switch(Opcode) {
> +    default: break;
> +  }
> +
> Added: llvm/trunk/lib/Target/PIC16/PIC16ISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PIC16/PIC16ISelLowering.cpp?rev=51027&view=auto
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Target/PIC16/PIC16ISelLowering.cpp (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16ISelLowering.cpp Tue May 13  
> 04:02:57 2008
> @@ -0,0 +1,801 @@
> +//===-- PIC16ISelLowering.cpp - PIC16 DAG Lowering Implementation  
> ---------===//
...
> +
> +const char *PIC16TargetLowering:: getTargetNodeName(unsigned  
> Opcode) const
> +{
> +  switch (Opcode)
> +  {
> +    case PIC16ISD::Hi        : return "PIC16ISD::Hi";
> +    case PIC16ISD::Lo        : return "PIC16ISD::Lo";
> +    case PIC16ISD::Package   : return "PIC16ISD::Package";
> +    case PIC16ISD::Wrapper   : return "PIC16ISD::Wrapper";
> +    case PIC16ISD::SetBank   : return "PIC16ISD::SetBank";
> +    case PIC16ISD::SetPage   : return "PIC16ISD::SetPage";
> +    case PIC16ISD::Branch    : return "PIC16ISD::Branch";
> +    case PIC16ISD::Cmp 	     : return "PIC16ISD::Cmp";

Spacing.

> +    case PIC16ISD::BTFSS     : return "PIC16ISD::BTFSS";
> +    case PIC16ISD::BTFSC     : return "PIC16ISD::BTFSC";
> +    case PIC16ISD::XORCC     : return "PIC16ISD::XORCC";
> +    case PIC16ISD::SUBCC     : return "PIC16ISD::SUBCC";
> +    default                  : return NULL;
> +  }
> +}
> +
> +PIC16TargetLowering::
> +PIC16TargetLowering(PIC16TargetMachine &TM): TargetLowering(TM)
> +{
> +  // PIC16 does not have i1 type, so use i8 for
> +  // setcc operations results (slt, sgt, ...).
> +  // setSetCCResultType(MVT::i8);
> +  // setSetCCResultContents(ZeroOrOneSetCCResult);
> +
Dead?

> +  // Store operations for i1 types must be promoted
> +  // setStoreXAction(MVT::i1, Promote);
> +  // setStoreXAction(MVT::i8, Legal);
> +  // setStoreXAction(MVT::i16, Custom);
> +  // setStoreXAction(MVT::i32, Expand);
> +
> +  // setOperationAction(ISD::BUILD_PAIR,     MVT::i32,  Expand);
> +  // setOperationAction(ISD::BUILD_PAIR,     MVT::i16,  Expand);
> +
Dead?

>
...
> +
> +SDOperand PIC16TargetLowering:: LowerOperation(SDOperand Op,  
> SelectionDAG &DAG)
> +{
> +  SDVTList VTList16 = DAG.getVTList(MVT::i16, MVT::i16, MVT::Other);
> +  switch (Op.getOpcode())
> +  {
> +    case ISD::STORE:

cout? or DOUT?
>
> +      cout << "reduce store\n";
> +	break;
> +    case ISD::FORMAL_ARGUMENTS:
> +      cout<<"==== lowering formal args\n";
> +      return LowerFORMAL_ARGUMENTS(Op, DAG);
> +    case ISD::GlobalAddress:
> +      cout<<"==== lowering GA\n";
> +      return LowerGlobalAddress(Op, DAG);
> +    case ISD::RET:      	
> +      cout<<"==== lowering ret\n";
> +      return LowerRET(Op, DAG);
> +    case ISD::FrameIndex:      	
> +      cout<<"==== lowering frame index\n";
> +      return LowerFrameIndex(Op, DAG);
> +    case ISD::ADDE:
> +      cout <<"==== lowering adde\n";
> +      break;
> +    case ISD::LOAD:
> +    case ISD::ADD:
> +      break;
> +    case ISD::BR_CC:		
> +      cout << "==== lowering BR_CC\n";
> +      return LowerBR_CC(Op, DAG);
> +  } //end swithch
> +  return SDOperand();
> +}
> +
...
> +    case ISD::SETNE:
> +    {
> +      cout << "setne\n";

cout or DOUT?

...
> +  // If this load is directly stored, replace the load value with  
> the stored
> +  // value.
> +  // TODO: Handle store large -> read small portion.
> +  // TODO: Handle TRUNCSTORE/LOADEXT

s/TODO/FIXME/

...

> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +// Target Optimization Hooks
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +SDOperand PIC16TargetLowering::PerformDAGCombine(SDNode *N,
> +                                                 DAGCombinerInfo  
> &DCI) const
> +{
> +  int i;
> +  ConstantSDNode *CST;
> +  SelectionDAG &DAG = DCI.DAG;
> +
> +  switch (N->getOpcode())
> +  {
> +  default: break;
> +  case PIC16ISD::Package  :
> +    cout <<"==== combining PIC16ISD::Package\n";

cout or DOUT?

...
> --- llvm/trunk/lib/Target/PIC16/PIC16InstrFormats.td (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16InstrFormats.td Tue May 13  
> 04:02:57 2008
> @@ -0,0 +1,112 @@
> +//===- PIC16RegisterInfo.td - PIC16 Register defs ------------*-  
> tblgen -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//  Describe PIC16 instructions format
> +//
> +//  All the possible PIC16 fields are:
> +//
> +//  opcode  - operation code.
> +//  f       - 7-bit register file address.
> +//  d       - 1-bit direction specifier
> +//  k       - 8/11 bit literals
> +//  b       - 3 bits bit num specifier
> +//
Could you use more descriptive names?

> --- llvm/trunk/lib/Target/PIC16/PIC16InstrInfo.cpp (added)
> +++ llvm/trunk/lib/Target/PIC16/PIC16InstrInfo.cpp Tue May 13  
> 04:02:57 2008
> @@ -0,0 +1,143 @@
> +//===- PIC16InstrInfo.cpp - PIC16 Instruction Information  
> -----------------===//
> +//
...
> +// TODO: Add the subtarget support on this constructor.

s/TODO/FIXME/

I have to finish here. Anyway, there are many similar things you might  
find in the rest of the code.

Thanks! :-)
-bw