[DAG] Prevent CombineTo from deleting already deleted nodes

[niravd]

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.

[davide]

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

[RKSimon]

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.

[niravd]

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]

Rewrite to avoid checking opcode of deleted nodes.

[niravd]

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

[niravd]

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?

[RKSimon]

LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

for (unsigned i = 0, e = NumTo; i != e; ++i) {
         AddUsersToWorklist(To[i].getNode());
       }
     }
   }
 
   // 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())
     deleteAndRecombine(N);
   return SDValue(N, 0);
 }
 
 void DAGCombiner::
 CommitTargetLoweringOpt(const TargetLowering::TargetLoweringOpt &TLO) {
   // Replace all uses.  If any nodes become isomorphic to other nodes and
   // are deleted, make sure to remove them from our worklist.
if (ISD::isNON_EXTLoad(N0.getNode()) && ISD::isUNINDEXEDLoad(N0.getNode()) &&
     if (DoXform) {
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, DL, VT, LN0->getChain(),
                                        LN0->getBasePtr(), N0.getValueType(),
                                        LN0->getMemOperand());
       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!
     }
   }
 
   // fold (sext (load x)) to multiple smaller sextloads.
   // Only on illegal but splittable vectors.
   if (SDValue ExtLoad = CombineExtLoad(N))
     return ExtLoad;
if (LN0->getExtensionType() != ISD::ZEXTLOAD && LN0->isUnindexed()) {
         APInt Mask = cast<ConstantSDNode>(N0.getOperand(1))->getAPIntValue();
         Mask = Mask.sext(VT.getSizeInBits());
         SDValue And = DAG.getNode(N0.getOpcode(), DL, VT,
                                   ExtLoad, DAG.getConstant(Mask, DL, VT));
         SDValue Trunc = DAG.getNode(ISD::TRUNCATE,
                                     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 =
               DAG.getNode(ISD::TRUNCATE, DL, N0.getValueType(), And);
           CombineTo(N0.getNode(), TruncAnd);
         }
         return CombineTo(N, And); // Return N so it doesn't get rechecked!
       }
if (DoXform) {
       SDValue ExtLoad = DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(N), VT,
                                        LN0->getChain(),
                                        LN0->getBasePtr(), N0.getValueType(),
                                        LN0->getMemOperand());
 
       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!
     }
   }
 
   // fold (zext (load x)) to multiple smaller zextloads.
   // Only on illegal but splittable vectors.
   if (SDValue ExtLoad = CombineExtLoad(N))
     return ExtLoad;
if (LN0->getExtensionType() != ISD::SEXTLOAD && LN0->isUnindexed()) {
         Mask = Mask.zext(VT.getSizeInBits());
         SDLoc DL(N);
         SDValue And = DAG.getNode(N0.getOpcode(), DL, VT,
                                   ExtLoad, DAG.getConstant(Mask, DL, VT));
         SDValue Trunc = DAG.getNode(ISD::TRUNCATE,
                                     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 =
               DAG.getNode(ISD::TRUNCATE, DL, N0.getValueType(), And);
           CombineTo(N0.getNode(), TruncAnd);
         }
         return CombineTo(N, And); // Return N so it doesn't get rechecked!
       }
if (ISD::isNON_EXTLoad(N0.getNode()) && !VT.isVector() &&
     if (!N0.hasOneUse())
       DoXform = ExtendUsesToFormExtLoad(N, N0, ISD::ANY_EXTEND, SetCCs, TLI);
     if (DoXform) {
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::EXTLOAD, SDLoc(N), VT,
                                        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!
     }
   }
 
   // fold (aext (zextload x)) -> (aext (truncate (zextload x)))
   // fold (aext (sextload x)) -> (aext (truncate (sextload x)))
   // fold (aext ( extload x)) -> (aext (truncate (extload  x)))
   if (N0.getOpcode() == ISD::LOAD &&
       !ISD::isNON_EXTLoad(N0.getNode()) && ISD::isUNINDEXEDLoad(N0.getNode()) &&

test/CodeGen/X86/pr32515.ll

+; 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
+
+@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
+ }