[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 12:42:16 PM
> Subject: Re: [PATCH] PEI scavengeFrameVirtualRegs and RS improvments
> 
> 
> On Mar 23, 2013, at 10:50 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> > Some details:
> > 
> > The existing code supports only one vreg -> physreg mapping at a
> > time, and as it processes the instruction operands in order, it
> > will find the definition of the new virtual register prior to the
> > use of the existing mapping. While this problem can be fixed by
> > scanning the operands in reverse order, that would not help the
> > case where we really have multiple virtual registers that should
> > be simultaneously live. To fix this, I removed the code which
> > maintained the explicit single mapping, and instead call
> > replaceRegWith as soon as the register mapping is established
> > (meaning that RS has scavenged a register). In some sense this
> > changes how the virtual frame scavenging could work: currently a
> > single virtual register, with multiple definitions (non-SSA),
> > could be reused in several places, and scavengeFrameVirtualRegs
> > will happily scavenge a new physical register for each definition.
> > As far as I can tell, only PPC and ARM are using virtual frame
> > scavenging, and neither of those backends use the functionality in
> > this way.
> 
> I think it would be a misuse of virtual registers to allow that. The
> machine code verifier also complains if the live range of a virtual
> register has multiple connected components. A single virtual
> register should only be used for a set of operands that must be
> allocated the same physical register.
> 
> I am not sure how clever scavengeFrameVirtualRegs() is, but you
> should probably make sure that the same scavenged register will be
> used to rewrite multiple virtual registers as long as their live
> ranges don't overlap and the scavenged register isn't clobbered in
> the meantime.
> 
> 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.

> 
> 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, once the above problem is solved, a code-quality
> > problem surfaces: the RS would never return a register killed by
> > some instruction when asked to scavenge a register at that
> > instruction. This would lead to more use of the spill slots than
> > necessary. To fix this issue I've done two things:
> > 
> > 1. In scavengeFrameVirtualRegs, RS->forward is called prior to
> > processing the instruction. This makes sure that killed registers
> > are marked as unused. forward() needed some small updates to
> > ignore the virtual registers.
> > 
> > 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?

Thanks again,
Hal

> 
> /jakob
> 
>