[llvm-commits] [llvm] r77897 - in /llvm/trunk: include/llvm/ADT/ lib/Support/ lib/Target/Blackfin/ lib/Target/Blackfin/AsmPrinter/ lib/Target/Blackfin/TargetInfo/ test/CodeGen/Blackfin/

[Chris Lattner]

On Aug 2, 2009, at 10:32 AM, Jakob Stoklund Olesen wrote:
> Author: stoklund
> Date: Sun Aug  2 12:32:10 2009
> New Revision: 77897
>
> Analog Devices Blackfin back-end.
>
> Generate code for the Blackfin family of DSPs from Analog Devices:
>
>  http://www.analog.com/en/embedded-processing-dsp/blackfin/processors/index.html

Very nice!  Here are my last round of nit-picky comments :)

> +
> +  // We must load delta into ScratchReg and add that.
> +  loadConstant(MBB, I, DL, ScratchReg, delta);
> +  if (BF::PRegClass.contains(Reg)) {
> +    assert (BF::PRegClass.contains(ScratchReg));

Minor syntax thing: please don't use a space after assert, assert is a  
function-like macro, not a control flow construct.

> +void BlackfinRegisterInfo::
> +eliminateCallFramePseudoInstr(MachineFunction &MF,
> +                              MachineBasicBlock &MBB,
> +                              MachineBasicBlock::iterator I) const {
> +  if (!hasReservedCallFrame(MF)) {
> +    int64_t Amount = I->getOperand(0).getImm();
> +    if (Amount != 0) {
> +      assert(Amount%4 == 0);

Also, it is good practice to add some sort of message to the asserts.   
Something like:

   assert(Amount%4 == 0 && "stack should be 4 byte aligned");

Would be good.

>
> +void BlackfinRegisterInfo::eliminateFrameIndex 
> (MachineBasicBlock::iterator II,
> +                                               int SPAdj,
> +                                               RegScavenger *RS)  
> const {
> +  unsigned i;

Please shrink the live range of i by declaring it right before it is  
used.  Also, it is best to only name things "i" if they are loop  
induction variables defined in the loop but not live out.  Since this  
is an important value used throughout the function, it would be best  
to name it something more evocative.

> +  MachineInstr &MI = *II;
> +  MachineBasicBlock &MBB = *MI.getParent();
> +  MachineFunction &MF = *MBB.getParent();
> +  DebugLoc DL = MI.getDebugLoc();
> +
> +  for (i=0; !MI.getOperand(i).isFI(); i++) {

Please use preincrement (++i).  Even though it doesn't matter for  
ints, it is good for consistency.

> +++ llvm/trunk/lib/Target/Blackfin/BlackfinRegisterInfo.h Sun Aug  2  
> 12:32:10 2009
> @@ -0,0 +1,114 @@
> +
> +  template<unsigned N>
> +  static inline bool isImm(int x) {
> +    return x >= -(1<<(N-1)) && x < (1<<(N-1));
> +  }
> +
> +  template<unsigned N>
> +  static inline bool isUimm(unsigned x) {
> +    return x < (1<<N);
> +  }

This looks like a generally handy set of templates, can you hoist  
these up to llvm/Support/MathExtras.h and turn them into templates on  
the integer type like:

   template<unsigned N, typename T>
   static inline bool isUimm(T x) {
     return x < ((T)1<<N);
   }

> +BlackfinTargetAsmInfo::BlackfinTargetAsmInfo() {
> +  GlobalPrefix = "_";
> +  CommentString = "//";

Ah, I knew we supported multicharacter comments for a reason ;-)

Thanks for building this!

-Chris