Page MenuHomePhabricator

Compilation crash after node replacement

Authored by delena on Nov 23 2016, 6:53 AM.



The bug is here:

The problem occurs when we take a node N and analyze its predecessors (operands of operands). We start from N and recursively go up N = N->getOperand(0).
On some stage of the climbing, we decide to replace one of predecessors with ReplaceAllUsesWith(N, NewNode). The ReplaceAllUsesWith works recursively and at some stage kills parent node.
I added ReplaceOneUseWith() to prevent recursive deletion.
Please let me know if this solution looks reasonable. (I attached the original reproducer, I'll minimize it if the solution itself makes sense.)

Diff Detail


Event Timeline

delena updated this revision to Diff 79066.Nov 23 2016, 6:53 AM
delena retitled this revision from to Compilation crash after node replacement.
delena updated this object.
delena added reviewers: zvi, craig.topper, RKSimon, spatel.
delena set the repository for this revision to rL LLVM.
delena added a subscriber: llvm-commits.
RKSimon edited edge metadata.Nov 23 2016, 7:08 AM

Please can you add context to the diff?

delena updated this revision to Diff 79073.Nov 23 2016, 7:12 AM
delena edited edge metadata.

Added context to the diff.

spatel edited edge metadata.Nov 27 2016, 8:54 AM

I haven't looked at this level of the DAG very much, so I'm not the best reviewer for the patch.
But if it's valid to specialize the one-use case, then can we just add a 'hasOneUse()' check to the existing ReplaceAllUsesWith() instead of adding another function call? That way, all callers of ReplaceAllUsesWith() would benefit. Or is there some other difference that triggers the bug that we're trying to avoid?

delena added inline comments.Nov 27 2016, 10:28 PM

The crash happens due to this line. RemoveNodeFromCSEMaps() - deletes node that is currently in-use.

Please reduce the test case as much as you can - remove the metadata and run it through bugpoint to isolate the issue.

I have a similar query to Sanjay: what is preventing you from having a 'hasOneUse' fast path inside SelectionDAG::ReplaceAllUsesWith - isn't a similar crash likely to occur in other cases?

Any movement on this?

RKSimon edited edge metadata.Sep 8 2017, 3:53 AM

@delena Abandon this? It doesn't look necessary any more

I've confirmed that PR31045 was fixed by D31286/rL298633, I've added a reduced version of 'addr-calc-crash.ll' at rL312786

delena abandoned this revision.Sep 29 2017, 4:43 AM