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

[Evan Cheng]

On Sep 25, 2009, at 12:47 AM, Jakob Stoklund Olesen wrote:

>
> 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.

Yes, this makes sense. RS cannot handle scavenging more than one  
register at a time. That means you just have to keep track the virtual  
register def that is live and look for a free physical register when  
it's killed (which should be the next instruction or else this  
violates the short live range assumption).

Evan

>
>> +    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.
>
>> +    }
>> +  }
>> +}
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits