[llvm-commits] [llvm] r82734 - in /llvm/trunk/lib: CodeGen/PrologEpilogInserter.cpp CodeGen/PrologEpilogInserter.h Target/ARM/ARMBaseRegisterInfo.cpp Target/ARM/Thumb1RegisterInfo.cpp

[Jakob Stoklund Olesen]

On 25/09/2009, at 01.52, Jim Grosbach wrote:
> Start of revamping the register scavenging in PEI. ARM Thumb1 is the  
> driving
> interest for this, as it currently reserves a register rather than  
> using
> the scavenger for matierializing constants as needed.

Great! Please see comments below.

> +void PEI::scavengeFrameVirtualRegs(MachineFunction &Fn) {
...
> +    // Keep a map of which scratch reg we use for each virtual reg.
> +    // FIXME: Is a map like this the best solution? Seems like  
> overkill,
> +    // but to get rid of it would need some fairly strong assumptions
> +    // that may not be valid as this gets smarter about reuse and  
> such.
> +    IndexedMap<unsigned, VirtReg2IndexFunctor> ScratchRegForVirtReg;

It seems that for now, we must require that live ranges of virtual  
registers are disjoint. Otherwise we would need more than one  
emergency spill slot in the worst case.

With only one virtual register live at a time, the map is indeed  
overkill.

It would also be a good idea to enforce the one-virtual-register-at-a- 
time rule with asserts here.

> +    ScratchRegForVirtReg.grow(Fn.getRegInfo().getLastVirtReg());
> +
> +    for (MachineBasicBlock::iterator I = BB->begin(); I != BB->end 
> (); ++I) {
> +      MachineInstr *MI = I;
> +      for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i)
> +        if (MI->getOperand(i).isReg()) {
> +          unsigned Reg = MI->getOperand(i).getReg();
> +          if (Reg && TRI->isVirtualRegister(Reg)) {

Would it perhaps be possible to reduce nesting with 'continue' here?

> +            // If we already have a scratch for this virtual  
> register, use it
> +            unsigned NewReg = ScratchRegForVirtReg[Reg];
> +            if (!NewReg) {
> +              const TargetRegisterClass *RC = Fn.getRegInfo 
> ().getRegClass(Reg);
> +              NewReg = RS->FindUnusedReg(RC);
> +              if (NewReg == 0)
> +                // No register is "free". Scavenge a register.
> +                // FIXME: Track SPAdj. Zero won't always be right
> +                NewReg = RS->scavengeRegister(RC, I, 0);

Ideally, I would like to get rid of FindUnusedReg(), so you could just  
say scavengeRegister() here. The ARMLoadStoreOptimizer is the only one  
to use FindUnusedReg() without immediately calling scavengerRegister()  
when it fails. I am no sure if that is intentional.

There is another issue here. FindUnusedReg() will give you a register  
that is unused /at the current position/. You really need a register  
that is unused in the whole live range of the virtual register you are  
replacing. scavengeRegister() won't return a register that is used by  
the instruction at I, but it looks like we need a variant that can  
exclude from whole range of instructions.

The rule should be: "NewReg must not be defined by any instruction in  
the range, except for the last. NewReg must not be an EarlyClobber  
operand on the last instruction."

> +              assert (NewReg && "unable to scavenge register!");
> +              ScratchRegForVirtReg[Reg] = NewReg;
> +            }
> +            MI->getOperand(i).setReg(NewReg);

For full generality, you need to watch out for subreg operands when  
replacing a virtreg with a physreg. At least you should assert 
(getSubReg()==0).

> +          }
> +        }
> +      RS->forward(MI);

Yeah, I also want to get rid of forward() as a public method. Since  
scavengeRegister takes an iterator argument that /must/ be next(MI)  
anyway, there is really no point to having it.

Batch forwarding over a range of instructions is going to be faster as  
well.

> +    }
> +  }
> +}