This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Avoid using deleted node in rebuildSetCC
ClosedPublic

Authored by niravd on May 3 2018, 12:35 PM.

Details

Summary

The combine in rebuildSetCC may be combined to another
node leaving our references stale. Keep a handle on
it to avoid stale references.

Fixes PR36602.

Diff Detail

Event Timeline

niravd created this revision.May 3 2018, 12:35 PM

This patch at least solves the problem with r325892 that I reported here:
https://bugs.llvm.org/show_bug.cgi?id=36602#c2

Thanks!

This works for me too and looks good (but again, I don't know the framework very well). Thanks!

efriedma added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11511

This needs a much better comment explaining what we're doing, and why; normally the DAGCombine visitors are not supposed to call each other (because it causes weird issues like this).


And actually, I'm not sure why we're calling visitXOR here in the first place; if we're missing some important optimization on xors, we should probably fix the setcc optimizations to do the same thing rather than try to delay the xor->setcc transform. But I won't ask you to change that here; I don't want to delay the bugfix.

niravd updated this revision to Diff 145952.May 9 2018, 10:15 AM

Add some explanatory text. Minor cleanup reducing number of handle construction/destructions.

efriedma accepted this revision.May 9 2018, 2:48 PM

LGTM

llvm/test/CodeGen/X86/pr36602.ll
22

This testcase seems fragile, but I guess it's hard to come up with anything better for DAGCombine.

This revision is now accepted and ready to land.May 9 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.