On Monday 01 March 2010 13:41:12 Dan Gohman wrote:> On Mar 1, 2010, at 7:26 AM, David Greene wrote: > >> Perhaps this can be fixed by making the code skip the ReplaceUses > >> call in the case where there are no uses to replace. That's not trivial > >> to detect though. > > > > Why not just check the same thing the added asserts check? > > You mean ->getOpcode() == ISD::DELETED_NODE? That's not fundamentally > any better, because if your purpose is to make this code work even > if nodes are actually deleted, that would still be a use of free'd > memory.Wait, I thought memory wasn't actually freed, just returned to a free list.> > UI goes bad and we blow up after returning from a deeply recursed call. > > > > It's simply not safe to iterate over a set that may change. > > Unfortunately, any of the nodes under the iterators may change so I don't > > see an easy way to fix this. > > The thing it's iterating over is a linked list. And the use_end() iterator > is essentially a null pointer.No, what I mean is the thing under UI at the point of call to AddModifiedNodeToCSEMaps gets deleted. So UI is invalid and when we loop back around and check it against UE we blow up with a singular iterator error. I can add code to save a few values of UI, find one that "works" after AddModifiedNodeToCSEMaps and get llc to finish. But that's a horrible hack only meant to prove that this is actually the problem. We're essentially doing this: std::list<...>::iterator i = ... for (... i != list.end(); ++i) { foo(i); } foo(iterator i) { list.erase(i); } -Dave
On Mar 1, 2010, at 1:59 PM, David Greene wrote:> On Monday 01 March 2010 13:41:12 Dan Gohman wrote: >> On Mar 1, 2010, at 7:26 AM, David Greene wrote: >>>> Perhaps this can be fixed by making the code skip the ReplaceUses >>>> call in the case where there are no uses to replace. That's not trivial >>>> to detect though. >>> >>> Why not just check the same thing the added asserts check? >> >> You mean ->getOpcode() == ISD::DELETED_NODE? That's not fundamentally >> any better, because if your purpose is to make this code work even >> if nodes are actually deleted, that would still be a use of free'd >> memory. > > Wait, I thought memory wasn't actually freed, just returned to a free list. > >>> UI goes bad and we blow up after returning from a deeply recursed call. >>> >>> It's simply not safe to iterate over a set that may change. >>> Unfortunately, any of the nodes under the iterators may change so I don't >>> see an easy way to fix this. >> >> The thing it's iterating over is a linked list. And the use_end() iterator >> is essentially a null pointer. > > No, what I mean is the thing under UI at the point of call to > AddModifiedNodeToCSEMaps gets deleted. So UI is invalid and when > we loop back around and check it against UE we blow up with a > singular iterator error.UI is incremented before AddModifiedNodeToCSEMaps is called, so I'm still not seeing what you're describing here. Dan
On Monday 01 March 2010 20:32:07 Dan Gohman wrote:> > No, what I mean is the thing under UI at the point of call to > > AddModifiedNodeToCSEMaps gets deleted. So UI is invalid and when > > we loop back around and check it against UE we blow up with a > > singular iterator error. > > UI is incremented before AddModifiedNodeToCSEMaps is called, so > I'm still not seeing what you're describing here.It's not the increment that trips, it's the loop top compare to UE. It doesn't matter where UI points at the call to AddModifiedNodeToCSEMaps. The fact that AddModifiedNodeToCSEMaps can recursively call ReplaceAllUsesOf means that we can potentially delete the node out from under UI. -Dave