my comments so far http://codereview.appspot.com/2781041/diff/1/include/llvm/Target/TargetInstrI... File include/llvm/Target/TargetInstrInfo.h (right): http://codereview.appspot.com/2781041/diff/1/include/llvm/Target/TargetInstrI... include/llvm/Target/TargetInstrInfo.h:70: template <class SUB, bool (SUB::*FUN)(MachineInstr*, MachineInstr*, missing doxygen comments around here http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.cpp File lib/Target/ARM/ARMBaseInstrInfo.cpp (right): http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1454: void *llvm::Opportunity::operator new(size_t need, Opportunity& space) { this should end up in a target-agnostic file http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1462: MachineBasicBlock::iterator &MII); //FIXME rearrange code so that forward referencing becomes unnecessary http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1467: int CmpValue; needed? http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1503: template <unsigned MAX> doxygenize http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1522: /// by chains of COPY instructions punctuation also explain a bit how it works http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1525: unsigned Eqiv; // FIXME uint32_t ? this can lead to slightly different behaviour on platforms with differing 'unsigned' bitsize, use uint32_t instead http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1596: new(Opp) ImmCmpOpportunity(MI->getOperand(0).getReg()); new (Opp) below too
Add Comments stated by Bill Wendling. http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.cpp File lib/Target/ARM/ARMBaseInstrInfo.cpp (left): http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1495: llvm::next(MachineBasicBlock::iterator(MI))); Bill's comment: leave this as llvm::next as it may conflict with std::next on some existing compilers http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.cpp File lib/Target/ARM/ARMBaseInstrInfo.cpp (right): http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1488: optimizeWith<MaskRegOpportunity, indentation! http://codereview.appspot.com/2781041/diff/1/lib/Target/ARM/ARMBaseInstrInfo.... lib/Target/ARM/ARMBaseInstrInfo.cpp:1751: CopyEquiv c1(TST->getOperand(0).getReg()); bad indentation