[PATCH] PEI scavengeFrameVirtualRegs and RS improvments

[Jakob Stoklund Olesen]

Looks good.

/jakob

On Mar 24, 2013, at 1:36 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
>> From: "Jakob Stoklund Olesen" <stoklund at 2pi.dk>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
>> Sent: Sunday, March 24, 2013 2:30:58 PM
>> Subject: Re: [PATCH] PEI scavengeFrameVirtualRegs and RS improvments
>> 
>> 
>> 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.
> 
> Alright, then we'll keep this behavior for now (I'll add a note about this somewhere).
> 
>> 
>>>> 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()).
> 
> Okay, this was simple enough to change; I like this way better (and requires less change). I've attached a revised patch (in which PEI calls scavengeRegister with llvm::next(I)). How's this?
> 
> Thanks again,
> Hal
> 
>> 
>> /jakob
>> 
>> 
> <pei_rs-v2.patch>