Page MenuHomePhabricator

[DAG] Fix Node Replacement in PromoteIntBinOp
ClosedPublic

Authored by niravd on Aug 10 2017, 8:04 AM.

Details

Summary

When one operand is a user of another in a promoted binary operation
we may replace and delete the returned value before returning
triggering an assertion. Reorder node replacements to prevent this.

Fixes PR34137.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Aug 10 2017, 8:04 AM
efriedma edited edge metadata.Aug 10 2017, 4:28 PM

This is why I hate DAGCombine; you have to constantly worry about the CSE map invalidating pointers you care about, and I'm never confident I'm handling it correctly.

Could we simplify this code using makeEquivalentMemoryOrdering to insert the loads into the right spot in the DAG? Or does that introduce its own RAUW problems?

This is why I hate DAGCombine; you have to constantly worry about the CSE map invalidating pointers you care about, and I'm never confident I'm handling it correctly.

Could we simplify this code using makeEquivalentMemoryOrdering to insert the loads into the right spot in the DAG? Or does that introduce its own RAUW problems?

No, this is a close but unrelated problem. This has been latent for a long while. I'm a little surprised that it required rL297695 to expose this. FWIW I've found that the way to deal with CSE invalidation from multiple replacements is to calculate which nodes need to be replaced upfront (either trivially or checking if there's more than one use) and replacing user nodes before their operands. Clunkier than I'd like, but straightforward. I also have a patch that more aggressively causes invalidation-based assertions where latent bugs may lie; I think it'll catch the lion share of the latent bugs.

Okay, that makes sense.

Is it possible for N0 to be an ancestor of N1? If it is, do we need to replace N1 before N0?

D'oh. It possible though hard. I'll add a cheap check and swap.

Okay, that makes sense.

Is it possible for N0 to be an ancestor of N1? If it is, do we need to replace N1 before N0?

niravd updated this revision to Diff 110790.Aug 11 2017, 12:55 PM

Add swap when both operands are exapnded loads w/ a dependence.

efriedma added inline comments.Aug 11 2017, 1:26 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1181 ↗(On Diff #110790)

You need to swap NN0 and NN1, I think?

dbabokin edited edge metadata.Aug 11 2017, 1:26 PM
I also have a patch that more aggressively causes invalidation-based assertions where latent bugs may lie; I think it'll catch the lion share of the latent bugs.

Let me know if you need to run assertion patch through random testing (which finds all this cases I'm filing as bugs).

niravd updated this revision to Diff 110831.Aug 11 2017, 7:03 PM

Add missing swap

RKSimon edited edge metadata.Aug 15 2017, 8:16 AM
I also have a patch that more aggressively causes invalidation-based assertions where latent bugs may lie; I think it'll catch the lion share of the latent bugs.

Let me know if you need to run assertion patch through random testing (which finds all this cases I'm filing as bugs).

Similarly, is there anything that we could add to EXPENSIVE_CHECKS builds to help identify these?

hans added a subscriber: hans.Aug 21 2017, 4:19 PM

Any progress here? PR34137 is marked as a release blocker.

hans added a comment.Aug 22 2017, 2:10 PM
In D36581#848166, @hans wrote:

Any progress here? PR34137 is marked as a release blocker.

Nirav: ping?

niravd marked an inline comment as done.Aug 23 2017, 9:08 AM

Missed line added. This should be okay. Can I get an LGTM?

efriedma accepted this revision.Aug 23 2017, 11:57 AM

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1180 ↗(On Diff #110831)

Calling isPredecessorOf is a little risky; it's usually cheap, but the cost blows up for large DAGs. Hopefully this doesn't cause problems in practice.

This revision is now accepted and ready to land.Aug 23 2017, 11:57 AM
hans added a comment.Aug 23 2017, 6:09 PM

Nirav asked me to land this since it's a release blocker and he's afk.

This revision was automatically updated to reflect the committed changes.