[llvm] r178845 - Fix bug in PEI's virtual-register scavenging
Hal Finkel
hfinkel at anl.gov
Fri Apr 5 14:33:03 PDT 2013
----- Original Message -----
> From: "Arnold Schwaighofer" <aschwaighofer at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>
> Sent: Friday, April 5, 2013 4:07:02 PM
> Subject: Re: [llvm] r178845 - Fix bug in PEI's virtual-register scavenging
>
> Hal this commit appears to have broken on of our LTO builds on
> spec/mesa. I get a segfault somewhere in the prolog epilog inserter
> that disappears if i revert this patch.
>
> Can you please revert this commit and I'll try to come up with a
> reduced test case in the meantime.
Reverted in r178916. Thanks!
-Hal
>
> On Apr 5, 2013, at 12:01 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> > Author: hfinkel
> > Date: Fri Apr 5 00:01:13 2013
> > New Revision: 178845
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=178845&view=rev
> > Log:
> > Fix bug in PEI's virtual-register scavenging
> >
> > This change fixes a bug that I introduced in r178058. After a
> > register is
> > scavenged using one of the available spills slots the instruction
> > defining the
> > virtual register needs to be moved to after the spill code. The
> > scavenger has
> > already processed the defining instruction so that registers killed
> > by that
> > instruction are available for definition in that same instruction.
> > Unfortunately,
> > after this, the scavenger needs to iterate through the spill code
> > and then
> > visit, again, the instruction that defines the now-scavenged
> > register. In order
> > to avoid confusion, the register scavenger needs the ability to
> > 'back up'
> > through the spill code so that it can again process the
> > instructions in the
> > appropriate order. Prior to this fix, once the scavenger reached
> > the
> > just-moved instruction, it would assert if it killed any registers
> > because,
> > having already processed the instruction, it believed they were
> > undefined.
> >
> > Unfortunately, I don't yet have a small test case. Thanks to Pranav
> > Bhandarkar
> > for diagnosing the problem and testing this fix.
> >
> > Modified:
> > llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h
> > llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp
> > llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
> >
> > Modified: llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h?rev=178845&r1=178844&r2=178845&view=diff
> > ==============================================================================
> > --- llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h (original)
> > +++ llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h Fri Apr 5
> > 00:01:13 2013
> > @@ -93,6 +93,15 @@ public:
> > while (MBBI != I) forward();
> > }
> >
> > + /// Invert the behavior of forward() on the current instruction
> > (undo the
> > + /// changes to the available registers made by forward()).
> > + void unprocess();
> > +
> > + /// Unprocess instructions until you reach the provided
> > iterator.
> > + void unprocess(MachineBasicBlock::iterator I) {
> > + while (MBBI != I) unprocess();
> > + }
> > +
> > /// skipTo - Move the internal MBB iterator but do not update
> > register states.
> > void skipTo(MachineBasicBlock::iterator I) {
> > if (I == MachineBasicBlock::iterator(NULL))
> > @@ -100,6 +109,10 @@ public:
> > MBBI = I;
> > }
> >
> > + MachineBasicBlock::iterator getCurrentPosition() const {
> > + return MBBI;
> > + }
> > +
> > /// getRegsUsed - return all registers currently in use in used.
> > void getRegsUsed(BitVector &used, bool includeReserved);
> >
> > @@ -171,6 +184,10 @@ private:
> > RegsAvailable |= Regs;
> > }
> >
> > + /// Processes the current instruction and fill the KillRegs and
> > DefRegs bit
> > + /// vectors.
> > + void determineKillsAndDefs();
> > +
> > /// Add Reg and all its sub-registers to BV.
> > void addRegWithSubRegs(BitVector &BV, unsigned Reg);
> >
> >
> > Modified: llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp?rev=178845&r1=178844&r2=178845&view=diff
> > ==============================================================================
> > --- llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp Fri Apr 5
> > 00:01:13 2013
> > @@ -826,6 +826,8 @@ void PEI::scavengeFrameVirtualRegs(Machi
> > for (MachineBasicBlock::iterator I = BB->begin(); I !=
> > BB->end(); ) {
> > MachineInstr *MI = I;
> > MachineBasicBlock::iterator J = llvm::next(I);
> > + MachineBasicBlock::iterator P = I == BB->begin() ?
> > + MachineBasicBlock::iterator(NULL) : llvm::prior(I);
> >
> > // RS should process this instruction before we might
> > scavenge at this
> > // location. This is because we might be replacing a virtual
> > register
> > @@ -869,8 +871,20 @@ void PEI::scavengeFrameVirtualRegs(Machi
> > // 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));
> > + BB->splice(J, BB, I);
> > +
> > + // Before we move I, we need to prepare the RS to visit I
> > again.
> > + // Specifically, RS will assert if it sees uses of
> > registers that
> > + // it believes are undefined. Because we have already
> > processed
> > + // register kills in I, when it visits I again, it will
> > believe that
> > + // those registers are undefined. To avoid this situation,
> > unprocess
> > + // the instruction I.
> > + assert(RS->getCurrentPosition() == I &&
> > + "The register scavenger has an unexpected position");
> > + I = P;
> > + RS->unprocess(P);
> > +
> > + // RS->skipTo(I == BB->begin() ? NULL : llvm::prior(I));
> > } else
> > ++I;
> > }
> >
> > Modified: llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegisterScavenging.cpp?rev=178845&r1=178844&r2=178845&view=diff
> > ==============================================================================
> > --- llvm/trunk/lib/CodeGen/RegisterScavenging.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/RegisterScavenging.cpp Fri Apr 5
> > 00:01:13 2013
> > @@ -110,30 +110,11 @@ void RegScavenger::addRegWithSubRegs(Bit
> > BV.set(*SubRegs);
> > }
> >
> > -void RegScavenger::forward() {
> > - // Move ptr forward.
> > - if (!Tracking) {
> > - MBBI = MBB->begin();
> > - Tracking = true;
> > - } else {
> > - assert(MBBI != MBB->end() && "Already past the end of the
> > basic block!");
> > - MBBI = llvm::next(MBBI);
> > - }
> > - assert(MBBI != MBB->end() && "Already at the end of the basic
> > block!");
> > +void RegScavenger::determineKillsAndDefs() {
> > + assert(Tracking && "Must be tracking to determine kills and
> > defs");
> >
> > MachineInstr *MI = MBBI;
> > -
> > - for (SmallVector<ScavengedInfo, 2>::iterator I =
> > Scavenged.begin(),
> > - IE = Scavenged.end(); I != IE; ++I) {
> > - if (I->Restore != MI)
> > - continue;
> > -
> > - I->Reg = 0;
> > - I->Restore = NULL;
> > - }
> > -
> > - if (MI->isDebugValue())
> > - return;
> > + assert(!MI->isDebugValue() && "Debug values have no kills or
> > defs");
> >
> > // Find out which registers are early clobbered, killed, defined,
> > and marked
> > // def-dead in this instruction.
> > @@ -167,6 +148,54 @@ void RegScavenger::forward() {
> > addRegWithSubRegs(DefRegs, Reg);
> > }
> > }
> > +}
> > +
> > +void RegScavenger::unprocess() {
> > + assert(Tracking && "Cannot unprocess because we're not
> > tracking");
> > +
> > + MachineInstr *MI = MBBI;
> > + if (MI->isDebugValue())
> > + return;
> > +
> > + determineKillsAndDefs();
> > +
> > + // Commit the changes.
> > + setUsed(KillRegs);
> > + setUnused(DefRegs);
> > +
> > + if (MBBI == MBB->begin()) {
> > + MBBI = MachineBasicBlock::iterator(NULL);
> > + Tracking = false;
> > + } else
> > + --MBBI;
> > +}
> > +
> > +void RegScavenger::forward() {
> > + // Move ptr forward.
> > + if (!Tracking) {
> > + MBBI = MBB->begin();
> > + Tracking = true;
> > + } else {
> > + assert(MBBI != MBB->end() && "Already past the end of the
> > basic block!");
> > + MBBI = llvm::next(MBBI);
> > + }
> > + assert(MBBI != MBB->end() && "Already at the end of the basic
> > block!");
> > +
> > + MachineInstr *MI = MBBI;
> > +
> > + for (SmallVector<ScavengedInfo, 2>::iterator I =
> > Scavenged.begin(),
> > + IE = Scavenged.end(); I != IE; ++I) {
> > + if (I->Restore != MI)
> > + continue;
> > +
> > + I->Reg = 0;
> > + I->Restore = NULL;
> > + }
> > +
> > + if (MI->isDebugValue())
> > + return;
> > +
> > + determineKillsAndDefs();
> >
> > // Verify uses and defs.
> > #ifndef NDEBUG
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
More information about the llvm-commits
mailing list