search for: replaceallusesof

On Thursday 25 February 2010 21:27:21 David A. Greene wrote: > Thanks. I don't have the source right in front of me. I'll take a look > when I can. Here's a fixed patch. It shows the same iterator errors I'm seeing with 2.5. Run make-check with a Debug+Checks compiler. I will file a bug once I have a proper internet connection. :-/ -Dave

...It shows the same iterator errors I'm seeing > with 2.5. Run make-check with a Debug+Checks compiler. I > will file a bug once I have a proper internet connection. :-/ Hmm, not the same exact error we're seeing. This one is in BlackfinISelDAGToDAG.cpp. Ours is in SelectionDAG::ReplaceAllUsesOf. But this is still a problem, I believe. The Blackfin people should look at it. I will keep investigating. -Dave

...ator errors I'm seeing > > with 2.5. Run make-check with a Debug+Checks compiler. I > > will file a bug once I have a proper internet connection. :-/ > > Hmm, not the same exact error we're seeing. This one is in > BlackfinISelDAGToDAG.cpp. Ours is in SelectionDAG::ReplaceAllUsesOf. I filed PR6434 for the Blackfin issue. Note that there is no Blackfin codegen component in Bugzilla. We probably want one. -Dave

...aps 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

...to get commit access then :( > > > Two questions: > > > > -- Any suggestions as to an existing test I could use as a model for > testing this? > > IS this what is causing this to be broken? > No no, nothing's broken. I just wanted to write some tests for the replaceAllUsesOf case, and wondered if there was an existing test to use as a model. > > $ cat t.ll > @union1 = constant union { i32*, i8 } { i32* @G } > @G = global i32 4 > $ llvm-as <t.ll | llvm-dis > ; ModuleID = '<stdin>' > > @union1 = constant union { i32*, i8 } undef...

...node has no uses. Right. > 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? What I'm seeing is a problem in ReplaceAllUsesOf itself. It recurses down and eventually replaces the node under the iterator in this use loop: SDNode::use_iterator UI = From.getNode()->use_begin(), UE = From.getNode()->use_end(); while (UI != UE) { SDNode *User = *UI; bool UserRemovedFromCSEMaps = false...

On Feb 16, 2010, at 12:38 PM, Talin wrote: > Here's the implementation of ConstantUnion::replaceUsesOfWithOnConstant(). No tests yet :( Seems basically ok, please commit. > > Two questions: > > -- Any suggestions as to an existing test I could use as a model for testing this? IS this what is causing this to be broken? $ cat t.ll @union1 = constant union { i32*, i8 } {

I just found a major bug in SelectionDAG. Well, I found it several weeks ago and finally diagnosed it today. One possible fix comes down to holding SDUses about to be processed in a queue. But this would require the SDUse copy constructor to be public. Why is it private and unimplemented right now? What's the assumption that's trying to protect?

SDUse's Prev and Next members implement a use list. Copying them probably wouldn't immediately break anything, but it wouldn't be meaningful. Dan On Feb 24, 2010, at 3:08 PM, David Greene wrote: > I just found a major bug in SelectionDAG. Well, I found it several weeks ago > and finally diagnosed it today. > > One possible fix comes down to holding SDUses about to be

...e. If I can come up with a solution that doesn't require a copy constructor I will of course use that. I can't get a reduced testcase that breaks on trunk but I do have some debug asserts that I'll try on trunk and see if I can get something to fail. There's an iterator issue with ReplaceAllUsesOf. -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. >>> >>>

...#39;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. Ok, I think I've finally managed to draw on my whiteboard a theoretical situation which could have a problem like this. The attached patch should theoretically fix this bug, though I have no way to confirm this right now. D...

...ng 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. > > What I'm seeing is a problem in ReplaceAllUsesOf itself. It recurses > down and eventually replaces the node under the iterator in this use > loop: > > SDNode::use_iterator UI = From.getNode()->use_begin(), > UE = From.getNode()->use_end(); > while (UI != UE) { > SDNode *User = *UI; >...

Here's the implementation of ConstantUnion::replaceUsesOfWithOnConstant(). No tests yet :( Two questions: -- Any suggestions as to an existing test I could use as a model for testing this? -- I could also use some advice on what part of the code to work on next (in terms of finishing up union support). -- -- Talin -------------- next part -------------- An HTML attachment was scrubbed...

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

...y 26 February 2010 10:34:41 David Greene wrote: >> On Friday 26 February 2010 09:55:32 David Greene wrote: >>> In the continuing quest to try to track down problems we're seeing >>> in >>> SelectionDAG, I added the following assert >>> toSelectionDAG::ReplaceAllUsesOfValuesWith: >> >> Here's a patch to add more of these deleted node asserts. They fire >> tons of times in the testbase. > > Ping? Just want to make sure this didn't get missed somehow. I'm > surprised to see no discussion. I've now looked at your lates...

On Friday 26 February 2010 10:34:41 David Greene wrote: > On Friday 26 February 2010 09:55:32 David Greene wrote: > > In the continuing quest to try to track down problems we're seeing in > > SelectionDAG, I added the following assert > > toSelectionDAG::ReplaceAllUsesOfValuesWith: > > Here's a patch to add more of these deleted node asserts. They fire > tons of times in the testbase. Ping? Just want to make sure this didn't get missed somehow. I'm surprised to see no discussion. -Dave

I see. Is there a pre-existing way to do this in LLVM? Cheers, ~Siddharth. On Thu, 17 Aug 2017 at 02:12 Craig Topper <craig.topper at gmail.com> wrote: > ConstantExprs are immutable, they can't be changed once they are created. > And a ConstantExpr can reference other ConstantExprs. So replacing all uses > of a Value in a ConstantExpr would require creating a new immutable