[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
clattner at apple.com
Sun Aug 2 23:38:31 PDT 2009
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
More information about the llvm-commits
mailing list