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

[Justin Bogner via llvm-commits]

LGTM!

Nirav Dave via Phabricator <reviews at reviews.llvm.org> writes:
> niravd updated this revision to Diff 105410.
> niravd added a comment.
>
> Rewrite to avoid checking opcode of deleted nodes.
>
>
> 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
> @@ -7134,7 +7134,11 @@
>        SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SDLoc(N0),
>                                    N0.getValueType(), ExtLoad);
>        ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::SIGN_EXTEND);
> -      CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
> +      // If the load value is used only by N, replace it via CombineTo N.
> +      if (N0.getValue(0).hasOneUse())
> +        DAG.ReplaceAllUsesOfValueWith(SDValue(N0, 1), ExtLoad.getValue(1));
> +      else
> +        CombineTo(LN0, Trunc, ExtLoad.getValue(1));
>        return CombineTo(N, ExtLoad); // Return N so it doesn't get rechecked!
>      }
>    }
> @@ -7192,7 +7196,11 @@
>                                      SDLoc(N0.getOperand(0)),
>                                      N0.getOperand(0).getValueType(), ExtLoad);
>          ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::SIGN_EXTEND);
> -        CombineTo(N0.getOperand(0).getNode(), Trunc, ExtLoad.getValue(1));
> +        // If the load value is used only by N, replace it via CombineTo N.
> +        if (SDValue(LN0, 0).hasOneUse())
> +          DAG.ReplaceAllUsesOfValueWith(SDValue(LN0, 1), ExtLoad.getValue(1));
> +        else
> +          CombineTo(LN0, Trunc, ExtLoad.getValue(1));
>          // If N0 has multiple uses, change other uses as well.
>          if (!N0.hasOneUse()) {
>            SDValue TruncAnd =
> @@ -7439,7 +7447,11 @@
>        SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SDLoc(N0),
>                                    N0.getValueType(), ExtLoad);
>        ExtendSetCCUses(SetCCs, Trunc, ExtLoad, SDLoc(N), ISD::ZERO_EXTEND);
> -      CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
> +      // If the load value is used only by N, replace it via CombineTo N.
> +      if (SDValue(LN0, 0).hasOneUse())
> +        DAG.ReplaceAllUsesOfValueWith(SDValue(LN0, 1), ExtLoad.getValue(1));
> +      else
> +        CombineTo(LN0, Trunc, ExtLoad.getValue(1));
>        return CombineTo(N, ExtLoad); // Return N so it doesn't get rechecked!
>      }
>    }
> @@ -7491,7 +7503,11 @@
>                                      SDLoc(N0.getOperand(0)),
>                                      N0.getOperand(0).getValueType(), ExtLoad);
>          ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::ZERO_EXTEND);
> -        CombineTo(N0.getOperand(0).getNode(), Trunc, ExtLoad.getValue(1));
> +        // If the load value is used only by N, replace it via CombineTo N.
> +        if (SDValue(LN0, 0).hasOneUse())
> +          DAG.ReplaceAllUsesOfValueWith(SDValue(LN0, 1), ExtLoad.getValue(1));
> +        else
> +          CombineTo(LN0, Trunc, ExtLoad.getValue(1));
>          // If N0 has multiple uses, change other uses as well.
>          if (!N0.hasOneUse()) {
>            SDValue TruncAnd =
> @@ -7670,13 +7686,16 @@
>                                         LN0->getChain(),
>                                         LN0->getBasePtr(), N0.getValueType(),
>                                         LN0->getMemOperand());
> -      CombineTo(N, ExtLoad);
>        SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SDLoc(N0),
>                                    N0.getValueType(), ExtLoad);
> -      CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
>        ExtendSetCCUses(SetCCs, Trunc, ExtLoad, SDLoc(N),
>                        ISD::ANY_EXTEND);
> -      return SDValue(N, 0);   // Return N so it doesn't get rechecked!
> +      // If the load value is used only by N, replace it via CombineTo N.
> +      if (N0.hasOneUse())
> +        DAG.ReplaceAllUsesOfValueWith(SDValue(LN0, 1), ExtLoad.getValue(1));
> +      else
> +        CombineTo(LN0, Trunc, ExtLoad.getValue(1));
> +      return CombineTo(N, ExtLoad); // Return N so it doesn't get rechecked!
>      }
>    }
>