[PATCH] PEI scavengeFrameVirtualRegs and RS improvments

[Hal Finkel]

----- 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
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pei_rs-v2.patch
Type: text/x-patch
Size: 5257 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130324/98dbd69b/attachment.bin>