[PATCH] PEI scavengeFrameVirtualRegs and RS improvments

[Jakob Stoklund Olesen]

On Mar 24, 2013, at 12:21 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> 
>> If you use replaceRegWith(), I don't think you'll know which
>> instruction is the last use, and you won't be setting kill flags.
>> Can the scavenger recover after that, or will it think that the
>> rewritten physreg stays alive because there are no kill flags?
> 
> Currently, code that uses the scavengeFrameVirtualRegs functionality is, as it must, manually setting the kill flags on the virtual registers; and these flags survive the register-replacement with the scavanged register. What I've done preserves this requirement.

OK, that works too.

>> Instead of replaceRegWith, you can steal a trick from RAFast: When
>> you rewrite virtual registers top-down, MRI.reg_empty(VirtReg) is
>> going to return true only after you've rewritten the last use of the
>> register. That's the kill, and you can set a kill flag on that last
>> use operand so the scavenger will be able to reuse the register.
> 
> I could do that. Maybe the best way of doing this is to make replaceRegWith set the kill flags (I could add a parameter to have it do this). Currently, the inner loop of replaceRegWith has:
> 
>    MachineOperand &O = I.getOperand();
>    ++I;
>    O.setReg(ToReg);
> 
> so making this (..., bool setKill = false):
> 
>    MachineOperand &O = I.getOperand();
>    ++I;
>    O.setReg(ToReg);
>    if (setKill && I == E)
>      O.setIsKill();
> 
> should accomplish what you've suggested, right?

Unfortunately, that won't work. The use-def list is not guaranteed to be ordered. (And globally live registers can have multiple kills. Or no kills.)

>>> 2. Normally, scavengeRegister will remove from consideration all
>>> registers referenced by the instruction pointed to by the
>>> iterator. I've added a boolean parameter to disable this behavior
>>> for killed registers (this cannot be made the default behavior
>>> because some code inserts code that uses the scavenged register
>>> prior to the provided iterator).
>> 
>> This sounds to me like a workaround for an off-by-one error.
>> Shouldn't it be properly defined if the scavenged register is safe
>> to use before or after the current scavenger position?
> 
> Hrmm... yes, it would be convenient (for me) to define that the returned register is free to be defined at the provided position. Both XCore and Hexagon call scavengeRegister and then insert definitions of the returned register using BuildMI at the same position (which I think inserts instructions prior to that position). I could update those targets to insert those instructions at llvm::next(I) instead. I'm afraid of subtly breaking out-of-tree targets with such a change.
> 
> On the other hand, maybe it is better if, in scavengeFrameVirtualRegs(), I call scavengeRegister with llvm::next(I) as the iterator. Because we're not actually inserting any instructions during scavengeFrameVirtualRegs, this might work as required.
> 
> What do you think?

It would make sense to me if scavengeRegister(I) returned a register that was available *before* the instruction at I, because BuildMI(I) inserts instructions *before* I. Then it would also makes sense to call scavengeRegister(MBB.end()).

/jakob