[llvm-commits] [PATCH] Analog Devices Blackfin back-end

[Chris Lattner]

On Jul 23, 2009, at 11:48 PM, Jakob Stoklund Olesen wrote:

> Hi,
>
> Finally my blackfin back-end is ready to be included in the main  
> tree. It is still experimental. I have compiled most of CodeGen/ 
> Generic as well as my own test cases. The blackfin assembler (bfin- 
> elf-as) is happy with the output, but I have not yet tested it on  
> real hardware. I have access to blackfin hardware, so I can do that  
> later. There is a GNU tool-chain at http://blackfin.uclinux.org/

Cool, the autoconf patch is fine, feel free to add it now if it won't  
break anything.

A "cumulative" patch is preferred, but the delta wasn't that big and  
I'm not trying to do a final review, just give some tips.

Some minor picky stuff:

+// Force static initialization.
+extern "C" void LLVMInitializeBlackfinAsmPrinter()
+{
+  TargetRegistry::RegisterAsmPrinter(TheBlackfinTarget,
+                                     createBlackfinCodePrinterPass);
+}

You also are inconsistent about putting the return type:

+SDNode*
+BlackfinDAGToDAGISel::Select(SDValue Op)
+{

Please put the name and return type on the same line if there is room,  
e.g.:

+SDNode *BlackfinDAGToDAGISel::Select(SDValue Op) {


Please use K&R style bracketing, where the { goes on the same line as  
the function name.

+  if (MI->getOperand(opNum+1).isImm()
+    && MI->getOperand(opNum+1).getImm() == 0)
+    return;

Please use && on the previous line, like so:

+  if (MI->getOperand(opNum+1).isImm() &&
+      MI->getOperand(opNum+1).getImm() == 0)
+    return;


Please consistently indent by 2:

+    default: return true;  // Unknown modifier.
+    case 'r':
+     break;
+    }

Please just use Bfin:: in isCC for consistency:

+static inline bool
+isCC(const TargetRegisterClass *RC)
+{
+  using namespace Bfin;
+  return RC == &AnyCCRegClass || AnyCCRegClass.hasSubClass(RC);
+}
+
+static inline bool
+isDCC(const TargetRegisterClass *RC)
+{
+  return RC == &Bfin::DRegClass || Bfin::DRegClass.hasSubClass(RC) ||  
isCC(RC);
+}

On that note, the Bfin target name space is kind of a weird  
contraction for it, I don't have a strong opinion on it either way,  
but maybe BF would be better?  Your call, do whatever you like.


Instead of:

+void
+BlackfinInstrInfo::storeRegToStackSlot(MachineBasicBlock &MBB,
...
+  if (inClass(Bfin::DPRegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE32fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
+  }
+  else if (inClass(Bfin::D16RegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE16fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
+  }
+  else if (inClass(Bfin::AnyCCRegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE8fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
+  }
+  else {
+    llvm_unreachable("Cannot store regclass to stack slot");
+  }

How about using early exits, giving something like:

+  if (inClass(Bfin::DPRegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE32fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
      return;
+  }
+  if (inClass(Bfin::D16RegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE16fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
      return;
+  }
+  if (inClass(Bfin::AnyCCRegClass, SrcReg, RC)) {
+    BuildMI(MBB, I, DL, get(Bfin::STORE8fi))
+      .addReg(SrcReg, getKillRegState(isKill))
+      .addFrameIndex(FI)
+      .addImm(0);
      return;
+  }
    llvm_unreachable("Cannot store regclass to stack slot");


likewise in loadRegFromStackSlot.

BlackfinRegisterInfo::emitPrologue looks like it could benefit from  
"early return".




Some more substantial comments:

+
+def LOAD8z32p_inc:      F1<(outs D:$dst, P:$ptr_wb), (ins P:$ptr),
+                           "$dst = B[$ptr++] (Z);", []>;
+def LOAD8z32p_dec:      F1<(outs D:$dst, P:$ptr_wb), (ins P:$ptr),
+                           "$dst = B[$ptr--] (Z);", []>;
+
+def LOAD8s32p_inc:      F1<(outs D:$dst, P:$ptr_wb), (ins P:$ptr),
+                           "$dst = B[$ptr++] (X);", []>;
+def LOAD8s32p_dec:      F1<(outs D:$dst, P:$ptr_wb), (ins P:$ptr),
+                           "$dst = B[$ptr--] (X);", []>;

It seems that bfin has a number of common "kinds" of instruction,  
could these benefit from a multipattern to factor them?

+def SRAi: F1<(outs D:$dst), (ins D:$src, i16imm:$amount),
+             "$dst >>>= $amount;",
+             [(set D:$dst, (sra D:$src, (i16 uimm5:$amount)))]>;
+
+def SRAr: F1<(outs D:$dst), (ins D:$src, D:$amount),
+             "$dst >>>= $amount;",
+             [(set D:$dst, (sra D:$src, D:$amount))]>;

likewise, handling r+i forms of instructions cries out for a  
multipattern.


Overall, this seems like a pretty clean and nice target in my quick  
first look!

-Chris