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

[Adrian Prantl]

Hmm.. This fails, too! Apparently the backend is really lax about enforcing sensible parent pointers.

diff --git a/lib/CodeGen/RegAllocFast.cpp b/lib/CodeGen/RegAllocFast.cpp
index 6e7e2c7..15f09f5 100644
--- a/lib/CodeGen/RegAllocFast.cpp
+++ b/lib/CodeGen/RegAllocFast.cpp
@@ -278,6 +278,7 @@ void RAFast::spillVirtReg(MachineBasicBlock::iterator MI,
                           LiveRegMap::iterator LRI) {
   LiveReg &LR = *LRI;
   assert(PhysRegState[LR.PhysReg] == LRI->VirtReg && "Broken RegState mapping");
+  assert(MI->getParent() == MBB && "MI is outside of MBB");
 
   if (LR.Dirty) {
     // If this physreg is used by the instruction, we want to kill it on the

 I wonder if that is the reason why the original code explicitly copied the parent pointer from the original intrinsic?

-- adrian

> On 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 <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> On Sep 5, 2014 10:22 AM, "Adrian Prantl" <aprantl at apple.com <mailto: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 <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.
>> > I failed at constructing a reliable testcase for this, see
>> > http://llvm.org/bugs/show_bug.cgi?id=20523 <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 <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.
> 
> 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 <mailto:llvm-commits at cs.uiuc.edu>
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
> 
> _______________________________________________
> 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/8dfb32d0/attachment.html>