[SDAG] Deal with deleted node in PromoteIntShiftOp

[niravd]

Summary

Deal with case that initial node is deleted during dag-combine leading
to an assertional failure in promoteIntShiftOp.

Fixes PR32420.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1156 ↗

(On Diff #93161)

These N0 arg changes should probably be done as a NFCI commit now

1172 ↗

(On Diff #93161)

if (Op && Op.getOpcode() != ISD::DELETED_NODE)

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1166–1169 ↗

(On Diff #93161)

Only the 'PromoteOperand' path can set 'Replace' to true, right? Can you move this into that else clause, and move the 'Replace' declaration into that else clause too?

test/CodeGen/X86/pr32420.ll

32–46 ↗

(On Diff #93161)

The bug is triggered by the 16-bit shifts. Can you remove the memops, globals, and struct?

[niravd]

Simplify test case and minor cleanup

[niravd]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1166–1169 ↗

(On Diff #93161)

Yes, but it would require us to duplicate the construction of RV which needs to be after the N0 change but before the replaceload call so in case N0 and N1 are closely related so we don't end up with a stale N1 value (another bug we've caught onto recently, e.g. https://reviews.llvm.org/D31148).

Given that I think this way is cleaner.

[RKSimon]

test/CodeGen/X86/pr32420.ll

29 ↗

(On Diff #93235)

please don't leave commented out instructions in the test case

[niravd]

Revert accidental comment and rebase

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1161 ↗

(On Diff #93245)

Is there any case where the promotion of N0 will affect N1? Wouldn't shift(N0, N0) do this?

[RKSimon]

No more comments from me - LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1161 ↗

(On Diff #93245)

Actually it seems we combine shift(N0, N0) --> 0

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (TLI.isTypeDesirableForOp(Opc, VT))
     return SDValue();
 
   EVT PVT = VT;
   // Consult target whether it is a good idea to promote this operation and
   // what's the right type to promote it to.
   if (TLI.IsDesirableToPromoteOp(Op, PVT)) {
     assert(PVT != VT && "Don't know what type to promote to!");
 
+    DEBUG(dbgs() << "\nPromoting "; Op.getNode()->dump(&DAG));
+
     bool Replace = false;
     SDValue N0 = Op.getOperand(0);
+    SDValue N1 = Op.getOperand(1);
     if (Opc == ISD::SRA)
       N0 = SExtPromoteOperand(N0, PVT);
     else if (Opc == ISD::SRL)
       N0 = ZExtPromoteOperand(N0, PVT);
     else
       N0 = PromoteOperand(N0, PVT, Replace);
+
     if (!N0.getNode())
       return SDValue();
 
+    SDLoc DL(Op);
+    SDValue RV =
+        DAG.getNode(ISD::TRUNCATE, DL, VT, DAG.getNode(Opc, DL, PVT, N0, N1));
+
     AddToWorklist(N0.getNode());
     if (Replace)
       ReplaceLoadWithPromotedLoad(Op.getOperand(0).getNode(), N0.getNode());
 
-    DEBUG(dbgs() << "\nPromoting ";
-          Op.getNode()->dump(&DAG));
-    SDLoc DL(Op);
-    return DAG.getNode(ISD::TRUNCATE, DL, VT,
-                       DAG.getNode(Opc, DL, PVT, N0, Op.getOperand(1)));
+    // Deal with Op being deleted.
+    if (Op && Op.getOpcode() != ISD::DELETED_NODE)
+      return RV;
   }
   return SDValue();
 }
 
 SDValue DAGCombiner::PromoteExtend(SDValue Op) {
   if (!LegalOperations)
     return SDValue();
 

llvm/trunk/test/CodeGen/X86/pr32420.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+
+@a = common local_unnamed_addr global i16 0, align 4
+@b = common local_unnamed_addr global i16 0, align 4
+
+define i32 @PR32420() {
+; CHECK-LABEL: PR32420:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    movq _a@{{.*}}(%rip), %rax
+; CHECK-NEXT:    movzwl (%rax), %eax
+; CHECK-NEXT:    movl %eax, %ecx
+; CHECK-NEXT:    shll $12, %ecx
+; CHECK-NEXT:    sarw $12, %cx
+; CHECK-NEXT:    movq _b@{{.*}}(%rip), %rdx
+; CHECK-NEXT:    movw %cx, %si
+; CHECK-NEXT:    orw (%rdx), %si
+; CHECK-NEXT:    andl %ecx, %esi
+; CHECK-NEXT:    movw %si, (%rdx)
+; CHECK-NEXT:    retq
+  %load2 = load i16, i16* @a, align 4
+  %shl3 = shl i16 %load2, 12
+  %ashr4 = ashr i16 %shl3, 12
+  %t2 = load volatile i16, i16* @b, align 4
+  %conv8 = or i16 %t2, %ashr4
+  %load9 = load i16, i16* @a, align 4
+  %shl10 = shl i16 %load9, 12
+  %ashr11 = ashr i16 %shl10, 12
+  %and = and i16 %conv8, %ashr11
+  store i16 %and, i16* @b, align 4
+  %cast1629 = zext i16 %load2 to i32
+  ret i32 %cast1629
+}