[PATCH] D34095: [DAG] Prevent CombineTo from deleting already deleted nodes

Justin Bogner via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 12:17:59 PDT 2017


Nirav Dave via Phabricator via llvm-commits
<llvm-commits at lists.llvm.org> writes:
> niravd created this revision.
>
> In combine passes where multiple nodes are updated, it is possible for
> an to-be-replaced node to be deleted and from a previous CombineTo
> call.  This causes a spurious node deletion. Add a check to
> prevent such deletions.
>
> Fixes PR32515.
>
>
> https://reviews.llvm.org/D34095
>
> Files:
>   lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>   test/CodeGen/X86/pr32515.ll
>
> Index: test/CodeGen/X86/pr32515.ll
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/X86/pr32515.ll
> @@ -0,0 +1,29 @@
> +; RUN: llc -O0 -mtriple=x86_64-unknown -mcpu=skx -o - %s
> +; RUN: llc     -mtriple=x86_64-unknown -mcpu=skx -o - %s
> +; RUN: llc -O0 -mtriple=i686-unknown   -mcpu=skx -o - %s
> +; RUN: llc     -mtriple=i686-unknown   -mcpu=skx -o - %s
> +; REQUIRES: asserts
> +
> + at var_26 = external global i16, align 2
> +
> +define void @foo() #0 {
> + %1 = alloca i16, align 2
> + %2 = load i16, i16* @var_26, align 2
> + %3 = zext i16 %2 to i32
> + %4 = icmp ne i32 %3, 7
> + %5 = zext i1 %4 to i16
> + store i16 %5, i16* %1, align 2
> + %6 = load i16, i16* @var_26, align 2
> + %7 = zext i16 %6 to i32
> + %8 = and i32 1, %7
> + %9 = shl i32 %8, 0
> + %10 = load i16, i16* @var_26, align 2
> + %11 = zext i16 %10 to i32
> + %12 = icmp ne i32 %11, 7
> + %13 = zext i1 %12 to i32
> + %14 = and i32 %9, %13
> + %15 = icmp ne i32 %14, 0
> + %16 = zext i1 %15 to i8
> + store i8 %16, i8* undef, align 1
> + unreachable
> + }
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -945,7 +945,7 @@
>    // Finally, if the node is now dead, remove it from the graph.  The node
>    // may not be dead if the replacement process recursively simplified to
>    // something else needing this node.
> -  if (N->use_empty())
> +  if (N->use_empty() && N->getOpcode() != ISD::DELETED_NODE)

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.

>      deleteAndRecombine(N);
>    return SDValue(N, 0);
>  }
>



More information about the llvm-commits mailing list