[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 4:08:40 PM
> Subject: Re: [PATCH] PEI scavengeFrameVirtualRegs and RS improvments
> 
> Looks good.

I committed this in r178058; but with a small change: The ++I at the end of the loop in PEI was replaced with:

+      // If the scavenger needed to use one of its spill slots, the
+      // spill code will have been inserted in between I and J. This is a
+      // problem because we need the spill code before I: Move I to just
+      // prior to J.
+      if (I != llvm::prior(J)) {
+        BB->splice(J, BB, I++);
+        RS->skipTo(I == BB->begin() ? NULL : llvm::prior(I));
+      } else
+        ++I;

where J is initialized at the top of the loop to be:

+      MachineBasicBlock::iterator J = llvm::next(I);

because calling scavengeRegister with llvm::next(I) causes spill code to be inserted before I, and that is not quite what we need in this case.

Thanks again,
Hal

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