This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Prevent CombineTo from deleting already deleted nodes
ClosedPublic

Authored by niravd on Jun 11 2017, 4:59 PM.

Details

Summary

When replacing a node and its operand, replacing the operand node may
cause the deletion of the original node leading to an assertion
failure. Case around these replacements to avoid this without relying
on inspecting the opcode of deleted nodes in various
DAGCombiner rewrites dealing with value extensions.

Fixes PR32515.

Event Timeline

niravd created this revision.Jun 11 2017, 4:59 PM
davide resigned from this revision.Jun 16 2017, 3:40 PM

I'm not familiar enough with the dag combiner to approve this patch. @RKSimon or somebody else should take a look.

RKSimon edited edge metadata.
RKSimon added a subscriber: chandlerc.

Adding @chandlerc who refactored this code at rL214623

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
951

Not sure if its relevant but there are several other places that call deleteAndRecombine on a node under similar circumstances.

Phabricator is still dropping comments from emails after the initial line. Reproducing here:

Justin Bogner said:

Can we do this without checking for ISD::DELETED_NODE? It's UB to read
N->getOpcode() if N was deleted, which we're currently working around by
unpoisoning the memory in ASAN, since we have this kind of code for
legacy reasons. I'd rather not introduce more places where we're doing
this.

niravd updated this revision to Diff 105410.Jul 6 2017, 5:54 AM

Rewrite to avoid checking opcode of deleted nodes.

niravd updated this revision to Diff 107119.Jul 18 2017, 9:34 AM

Rebase and remove dependent patch in effort to land this soon.

The previous version patch was LGTMed by bogner by email about a week ago (but was dropped by Phabricator). I've rebased and dropped the dependence on PR35030 exposing the minor degradation in avx512-mask-op.ll which is an exposed variation from reordering sensitivity. This gets resolved by PR35030,

Can someone give me an official accept on this?

This revision is now accepted and ready to land.Jul 18 2017, 10:24 AM
This revision was automatically updated to reflect the committed changes.