[llvm-commits] [llvm] r40521 - in /llvm/trunk: include/llvm/CodeGen/Passes.h lib/CodeGen/LLVMTargetMachine.cpp lib/CodeGen/LowerSubregs.cpp

[Chris Lattner]

On Jul 26, 2007, at 1:18 AM, Christopher Lamb wrote:

> Author: clamb
> Date: Thu Jul 26 03:18:32 2007
> New Revision: 40521

Nice work on subregs!

> +  /// LowerSubregs Pass - This pass lowers subregs to register- 
> register copies
> +  /// which yields suboptimial, but correct code if the register  
> allocator

suboptimial -> suboptimal


> +// Returns the Register Class of a physical register
> +static const TargetRegisterClass *getPhysicalRegisterRegClass(
> +        const MRegisterInfo &MRI,
> +        unsigned reg) {

Please end comments with a period if they are a sentence.

> +  assert(MRegisterInfo::isPhysicalRegister(reg) &&
> +         "reg must be a physical register");
> +  // Pick the register class of the right type that contains this  
> physreg.
> +  for (MRegisterInfo::regclass_iterator I = MRI.regclass_begin(),
> +         E = MRI.regclass_end(); I != E; ++I)
> +    if ((*I)->contains(reg))
> +      return *I;
> +  assert(false && "Couldn't find the register class");
> +  return 0;
> +}
> +
> +static bool isSubRegOf(const MRegisterInfo &MRI,
> +                       unsigned SubReg,
> +                       unsigned SupReg) {
> +  const TargetRegisterDesc &RD = MRI[SubReg];
> +  for (const unsigned *reg = RD.SuperRegs; *reg != 0; ++reg)
> +    if (*reg == SupReg)
> +      return true;
> +
> +  return false;
> +}

Would it make sense for getPhysicalRegisterRegClass/isSubRegOf to be  
methods on MRegisterInfo?

> +/// runOnMachineFunction - Reduce subregister inserts and extracts  
> to register
> +/// copies.
> +///
> +bool LowerSubregsInstructionPass::runOnMachineFunction 
> (MachineFunction &MF) {
> +  DOUT << "Machine Function\n";
> +  const TargetMachine &TM = MF.getTarget();
> +  const MRegisterInfo &MRI = *TM.getRegisterInfo();
> +
> +  bool MadeChange = false;
> +
> +  DOUT << "********** LOWERING SUBREG INSTRS **********\n";
> +  DOUT << "********** Function: " << MF.getFunction()->getName()  
> << '\n';
> +
> +  for (MachineFunction::iterator mbbi = MF.begin(), mbbe = MF.end();
> +       mbbi != mbbe; ++mbbi) {
> +    for (MachineBasicBlock::iterator mi = mbbi->begin(), me = mbbi- 
> >end();
> +         mi != me; ++mi) {
> +
> +      if (mi->getOpcode() == TargetInstrInfo::EXTRACT_SUBREG) {
...
> +
> +        DOUT << "\n";
> +        mbbi->erase(mi);
> +        MadeChange = true;
> +
> +      } else if (mi->getOpcode() == TargetInstrInfo::INSERT_SUBREG) {

...

> +
> +        DOUT << "\n";
> +        mbbi->erase(mi);
> +        MadeChange = true;
> +      }
> +    }
> +  }
> +
> +  return MadeChange;
> +}

This loop is reading from invalidated iterators.  Specifically, after  
you erase the machine instrs, the next iteration of the loop  
increments the "mi" iterator, which points to the deleted instruction.

I suggest structuring the loop like this:

   for (MachineFunction::iterator mbbi = MF.begin(), mbbe = MF.end();
        mbbi != mbbe; ++mbbi) {
     for (MachineBasicBlock::iterator mi = mbbi->begin(), me = mbbi- 
 >end();
          mi != me; ) {
       MachineInstr *MI = mi++;

       if (MI->getOpcode() == TargetInstrInfo::EXTRACT_SUBREG) {
         LowerExtract(MI);
       } else if (MI->getOpcode() == TargetInstrInfo::INSERT_SUBREG) {
         LowerInsert(MI);
       }
   }}

By incrementing the iterator early, you avoid the invalidation  
problems.  Use of methods for the lowering make the code easier to  
read but has no functionality change,

-Chris