[llvm] r217260 - Set the parent pointer of cloned DBG_VALUE instructions correctly.

[David Blaikie]

On Fri, Sep 5, 2014 at 1:13 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Sep 5, 2014, at 10:31 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Sep 5, 2014 10:22 AM, "Adrian Prantl" <aprantl at apple.com> wrote:
> >
> > Author: adrian
> > Date: Fri Sep  5 12:10:10 2014
> > New Revision: 217260
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=217260&view=rev
> > Log:
> > Set the parent pointer of cloned DBG_VALUE instructions correctly.
> > Fixes PR20523.
> >
> > When spilling variables onto the stack, spillVirtReg() is setting the
> > parent pointer of the cloned DBG_VALUE intrinsic for the stack location
> > to the parent pointer of the original intrinsic. MachineInstr parent
> > pointers should however always point to the parent basic block.
> >
> > MBB is shadowing the MBB member variable. The instruction still ends up
> > being inserted into the right basic block, because it's inserted after MI
> > which serves as the iterator.
>
> Perhaps we should assert that the iterator within the basic block?
>
> That makes sense on its own, but it wouldn’t help with catching the bug in
> the PR. I’ll add this as an additional assertion.
>

Hmm, why not? Wouldn't that assertion fail - since the MI is not in the MBB
in this case (before your patch)?


>
> > I failed at constructing a reliable testcase for this, see
> > http://llvm.org/bugs/show_bug.cgi?id=20523 for a large testcases.
> >
> > Modified:
> >     llvm/trunk/lib/CodeGen/RegAllocFast.cpp
> >
> > Modified: llvm/trunk/lib/CodeGen/RegAllocFast.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocFast.cpp?rev=217260&r1=217259&r2=217260&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/CodeGen/RegAllocFast.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/RegAllocFast.cpp Fri Sep  5 12:10:10 2014
> > @@ -309,10 +309,10 @@ void RAFast::spillVirtReg(MachineBasicBl
> >          DL = (--EI)->getDebugLoc();
> >        } else
> >          DL = MI->getDebugLoc();
> > -      MachineBasicBlock *MBB = DBG->getParent();
> >        MachineInstr *NewDV =
> >            BuildMI(*MBB, MI, DL, TII->get(TargetOpcode::DBG_VALUE))
> >                .addFrameIndex(FI).addImm(Offset).addMetadata(MDPtr);
> > +      assert(NewDV->getParent() == MBB && "dangling parent pointer");
>
> Maybe this assertion (in some form) could be moved into a more central
> location so this invariant (that BuildMI functions are passed an iterator
> into the same MBB - but maybe even below the build function, in the
> insertion itself)
>
>
> I experimented with the following patch and it seems to lead into a world
> of pain... Apparently we violate this invariant quite often at the moment.
>

What kind of violations? Is the parent just null here, perhaps? but even
then, seems like something not good.


>
> *diff --git a/include/llvm/CodeGen/MachineBasicBlock.h
> b/include/llvm/CodeGen/MachineBasicBlock.h*
> *index a08cc2e..edd228a 100644*
> *--- a/include/llvm/CodeGen/MachineBasicBlock.h*
> *+++ b/include/llvm/CodeGen/MachineBasicBlock.h*
> @@ -486,11 +486,13 @@ public:
>    /// Insert a range of instructions into the instruction list before I.
>    template<typename IT>
>    void insert(iterator I, IT S, IT E) {
> +    assert(I->getParent() == this && "iterator points outside of basic
> block");
>      Insts.insert(I.getInstrIterator(), S, E);
>    }
>
>
>    /// Insert MI into the instruction list before I.
>    iterator insert(iterator I, MachineInstr *MI) {
> +    assert(I->getParent() == this && "iterator points outside of basic
> block");
>      assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
>             "Cannot insert instruction with bundle flags");
>      return Insts.insert(I.getInstrIterator(), MI);
> @@ -498,6 +500,7 @@ public:
>
>
>    /// Insert MI into the instruction list after I.
>    iterator insertAfter(iterator I, MachineInstr *MI) {
> +    assert(I->getParent() == this && "iterator points outside of basic
> block");
>      assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
>             "Cannot insert instruction with bundle flags");
>      return Insts.insertAfter(I.getInstrIterator(), MI);
>
>
>
> >        (void)NewDV;
> >        DEBUG(dbgs() << "Inserting debug info due to spill:" << "\n" <<
> *NewDV);
> >      }
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140905/26107af8/attachment.html>