[SDAG] Avoid deleted SDNodes PromoteIntBinOp

[niravd]

Summary

Reorder work in PromoteIntBinOp to prevent stale (deleted) nodes from
being used.

Fixes PR32340 and PR32345

[niravd]

Catch missing case and add related test

[niravd]

Readd promotion failure check

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1099

What about cases where PromoteOperand returns SDValue()? Or where N0 == N1?

[RKSimon]

test/CodeGen/X86/pr32345.ll

2

There are failures both on -mtriple=i686-unknown and -mtriple=x86_64-unknown (different assertions, same cause) - please can you test both?

[niravd]

Fix failed promote case

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1099

The failure of PromoteOperand is an issue (fixed in new patch). If N0 == N1 (or is a subexpression), the replacement would be invalid and will not happen.

The SDValue check needs to be added.

[RKSimon]

test/CodeGen/X86/pr32340.ll

3

Please can you reduce and clean (strip metadata etc.) this test?

[niravd]

Simplify/cleanup tests and add Sanity Filecheck

[niravd]

Change guard to reflect actual deleted check.

[RKSimon]

LGTM but if you can please strip the *.getNode() from the bool checks - they're not necessary

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

1105

No need to include the .getNode() for the bool test

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 Replace0 = false;
     SDValue N0 = Op.getOperand(0);
     SDValue NN0 = PromoteOperand(N0, PVT, Replace0);
-    if (!NN0.getNode())
-      return SDValue();
 
     bool Replace1 = false;
     SDValue N1 = Op.getOperand(1);
-    SDValue NN1;
-    if (N0 == N1)
-      NN1 = NN0;
-    else {
-      NN1 = PromoteOperand(N1, PVT, Replace1);
-      if (!NN1.getNode())
-        return SDValue();
-    }
+    SDValue NN1 = PromoteOperand(N1, PVT, Replace1);
+    SDLoc DL(Op);
 
-    AddToWorklist(NN0.getNode());
-    if (NN1.getNode())
-      AddToWorklist(NN1.getNode());
+    SDValue RV =
+        DAG.getNode(ISD::TRUNCATE, DL, VT, DAG.getNode(Opc, DL, PVT, NN0, NN1));
 
-    if (Replace0)
+    // New replace instances of N0 and N1
+    if (Replace0 && N0.getNode() && N0.getOpcode() != ISD::DELETED_NODE &&
+        NN0.getNode() && NN0.getOpcode() != ISD::DELETED_NODE) {
+      AddToWorklist(NN0.getNode());
       ReplaceLoadWithPromotedLoad(N0.getNode(), NN0.getNode());
-    if (Replace1)
+    }
+
+    if (Replace1 && N1.getNode() && N1.getOpcode() != ISD::DELETED_NODE &&
+        NN1.getNode() && NN1.getOpcode() != ISD::DELETED_NODE) {
+      AddToWorklist(NN1.getNode());
       ReplaceLoadWithPromotedLoad(N1.getNode(), NN1.getNode());
+    }
 
-    DEBUG(dbgs() << "\nPromoting ";
-          Op.getNode()->dump(&DAG));
-    SDLoc DL(Op);
-    return DAG.getNode(ISD::TRUNCATE, DL, VT,
-                       DAG.getNode(Opc, DL, PVT, NN0, NN1));
+    if (NN0.getNode() && NN1.getNode())
+      return RV;
   }
   return SDValue();
 }
 
 /// Promote the specified integer shift operation if the target indicates it is
 /// beneficial. e.g. On x86, it's usually better to promote i16 operations to
 /// i32 since i16 instructions are longer.
 SDValue DAGCombiner::PromoteIntShiftOp(SDValue Op) {

test/CodeGen/X86/pr32340.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O0 -mtriple=x86_64-unknown-linux-gnu -o - %s | FileCheck %s -check-prefix=X64
+
+@var_825 = external global i16, align 2
+@var_32 = external global i16, align 2
+@var_901 = external global i16, align 2
+@var_826 = external global i64, align 8
+@var_57 = external global i64, align 8
+@var_900 = external global i16, align 2
+@var_28 = external constant i64, align 8
+@var_827 = external global i16, align 2
+
+define void @foo() {
+; X64-LABEL: foo:
+; X64:       # BB#0: # %entry
+; X64-NEXT:    movw $0, {{.*}}(%rip)
+; X64-NEXT:    movzwl {{.*}}(%rip), %eax
+; X64-NEXT:    movw %ax, %cx
+; X64-NEXT:    movw {{.*}}(%rip), %dx
+; X64-NEXT:    xorw %dx, %cx
+; X64-NEXT:    # implicit-def: %ESI
+; X64-NEXT:    movw %cx, %si
+; X64-NEXT:    movl %eax, %edi
+; X64-NEXT:    xorl %esi, %edi
+; X64-NEXT:    movw %di, %cx
+; X64-NEXT:    movzwl %cx, %esi
+; X64-NEXT:    movl %esi, %edi
+; X64-NEXT:    addl %eax, %edi
+; X64-NEXT:    movl %edi, %r8d
+; X64-NEXT:    movq %r8, {{.*}}(%rip)
+; X64-NEXT:    xorl $-772157262, %esi # imm = 0xD1F9D0B2
+; X64-NEXT:    movl {{.*}}(%rip), %eax
+; X64-NEXT:    movl %esi, %edi
+; X64-NEXT:    orl %eax, %edi
+; X64-NEXT:    orl %edi, %esi
+; X64-NEXT:    movw %si, %cx
+; X64-NEXT:    movw %cx, {{.*}}(%rip)
+; X64-NEXT:    movq {{.*}}(%rip), %r8
+; X64-NEXT:    testq %r8, %r8
+; X64-NEXT:    setne %r9b
+; X64-NEXT:    movzbl %r9b, %eax
+; X64-NEXT:    movw %ax, %cx
+; X64-NEXT:    movw %cx, var_827
+; X64-NEXT:    retq
+entry:
+  store i16 0, i16* @var_825, align 2
+  %v0 = load i16, i16* @var_32, align 2
+  %conv = zext i16 %v0 to i32
+  %v2 = load i16, i16* @var_901, align 2
+  %conv2 = zext i16 %v2 to i32
+  %xor = xor i32 %conv, %conv2
+  %xor3 = xor i32 %conv, %xor
+  %add = add nsw i32 %xor3, %conv
+  %conv5 = sext i32 %add to i64
+  store i64 %conv5, i64* @var_826, align 8
+  %v4 = load i16, i16* @var_32, align 2
+  %conv6 = zext i16 %v4 to i64
+  %v6 = load i16, i16* @var_901, align 2
+  %conv8 = zext i16 %v6 to i32
+  %xor9 = xor i32 51981, %conv8
+  %conv10 = sext i32 %xor9 to i64
+  %xor11 = xor i64 -1142377792914660288, %conv10
+  %xor12 = xor i64 %conv6, %xor11
+  %neg = xor i64 %xor12, -1
+  %xor13 = xor i64 %conv6, %neg
+  %v9 = load i16, i16* @var_901, align 2
+  %v10 = load i64, i64* @var_57, align 8
+  %or = or i64 %xor13, %v10
+  %or23 = or i64 %xor13, %or
+  %conv24 = trunc i64 %or23 to i16
+  store i16 %conv24, i16* @var_900, align 2
+  %v11 = load i64, i64* @var_28, align 8
+  %cmp = icmp ne i64 0, %v11
+  %conv25 = zext i1 %cmp to i16
+  store i16 %conv25, i16* @var_827, align 2
+  ret void
+}

test/CodeGen/X86/pr32345.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O0 -mtriple=x86_64-unknown-linux-gnu -o - %s | FileCheck %s -check-prefix=X640
+; RUN: llc -O0 -mtriple=i686-unknown             -o - %s | FileCheck %s -check-prefix=6860
+; RUN: llc     -mtriple=x86_64-unknown-linux-gnu -o - %s | FileCheck %s -check-prefix=X64
+; RUN: llc     -mtriple=i686-unknown             -o - %s | FileCheck %s -check-prefix=686
+
+@var_22 = external global i16, align 2
+@var_27 = external global i16, align 2
+
+define void @foo() {
+; X640-LABEL: foo:
+; X640:       # BB#0: # %bb
+; X640-NEXT:    # implicit-def: %RAX
+; X640-NEXT:    movzwl var_22, %ecx
+; X640-NEXT:    movzwl var_27, %edx
+; X640-NEXT:    xorl %edx, %ecx
+; X640-NEXT:    movzwl var_27, %edx
+; X640-NEXT:    xorl %edx, %ecx
+; X640-NEXT:    movslq %ecx, %rsi
+; X640-NEXT:    movq %rsi, -{{[0-9]+}}(%rsp)
+; X640-NEXT:    movzwl var_22, %ecx
+; X640-NEXT:    movzwl var_27, %edx
+; X640-NEXT:    xorl %edx, %ecx
+; X640-NEXT:    movzwl var_27, %edx
+; X640-NEXT:    xorl %edx, %ecx
+; X640-NEXT:    movslq %ecx, %rsi
+; X640-NEXT:    movzwl var_27, %ecx
+; X640-NEXT:    subl $16610, %ecx # imm = 0x40E2
+; X640-NEXT:    movl %ecx, %ecx
+; X640-NEXT:    # kill: %RCX<def> %ECX<kill>
+; X640-NEXT:    # kill: %CL<def> %RCX<kill>
+; X640-NEXT:    sarq %cl, %rsi
+; X640-NEXT:    movb %sil, %cl
+; X640-NEXT:    movb %cl, (%rax)
+; X640-NEXT:    retq
+;
+; 6860-LABEL: foo:
+; 6860:       # BB#0: # %bb
+; 6860-NEXT:    pushl %ebp
+; 6860-NEXT:  .Lcfi0:
+; 6860-NEXT:    .cfi_def_cfa_offset 8
+; 6860-NEXT:  .Lcfi1:
+; 6860-NEXT:    .cfi_offset %ebp, -8
+; 6860-NEXT:    movl %esp, %ebp
+; 6860-NEXT:  .Lcfi2:
+; 6860-NEXT:    .cfi_def_cfa_register %ebp
+; 6860-NEXT:    pushl %ebx
+; 6860-NEXT:    pushl %edi
+; 6860-NEXT:    pushl %esi
+; 6860-NEXT:    andl $-8, %esp
+; 6860-NEXT:    subl $32, %esp
+; 6860-NEXT:  .Lcfi3:
+; 6860-NEXT:    .cfi_offset %esi, -20
+; 6860-NEXT:  .Lcfi4:
+; 6860-NEXT:    .cfi_offset %edi, -16
+; 6860-NEXT:  .Lcfi5:
+; 6860-NEXT:    .cfi_offset %ebx, -12
+; 6860-NEXT:    # implicit-def: %EAX
+; 6860-NEXT:    movw var_22, %cx
+; 6860-NEXT:    movzwl var_27, %edx
+; 6860-NEXT:    movw %dx, %si
+; 6860-NEXT:    xorw %si, %cx
+; 6860-NEXT:    # implicit-def: %EDI
+; 6860-NEXT:    movw %cx, %di
+; 6860-NEXT:    xorl %edx, %edi
+; 6860-NEXT:    movw %di, %cx
+; 6860-NEXT:    movzwl %cx, %edi
+; 6860-NEXT:    movl %edi, {{[0-9]+}}(%esp)
+; 6860-NEXT:    movl $0, {{[0-9]+}}(%esp)
+; 6860-NEXT:    addl $-16610, %edx # imm = 0xBF1E
+; 6860-NEXT:    movb %dl, %bl
+; 6860-NEXT:    xorl %edx, %edx
+; 6860-NEXT:    movb %bl, %cl
+; 6860-NEXT:    shrdl %cl, %edx, %edi
+; 6860-NEXT:    testb $32, %bl
+; 6860-NEXT:    movl %eax, {{[0-9]+}}(%esp) # 4-byte Spill
+; 6860-NEXT:    movl %edi, {{[0-9]+}}(%esp) # 4-byte Spill
+; 6860-NEXT:    movl %edx, {{[0-9]+}}(%esp) # 4-byte Spill
+; 6860-NEXT:    jne .LBB0_2
+; 6860-NEXT:  # BB#1: # %bb
+; 6860-NEXT:    movl {{[0-9]+}}(%esp), %eax # 4-byte Reload
+; 6860-NEXT:    movl %eax, {{[0-9]+}}(%esp) # 4-byte Spill
+; 6860-NEXT:  .LBB0_2: # %bb
+; 6860-NEXT:    movl {{[0-9]+}}(%esp), %eax # 4-byte Reload
+; 6860-NEXT:    movb %al, %cl
+; 6860-NEXT:    movl {{[0-9]+}}(%esp), %eax # 4-byte Reload
+; 6860-NEXT:    movb %cl, (%eax)
+; 6860-NEXT:    leal -12(%ebp), %esp
+; 6860-NEXT:    popl %esi
+; 6860-NEXT:    popl %edi
+; 6860-NEXT:    popl %ebx
+; 6860-NEXT:    popl %ebp
+; 6860-NEXT:    retl
+;
+; X64-LABEL: foo:
+; X64:       # BB#0: # %bb
+; X64-NEXT:    movzwl {{.*}}(%rip), %ecx
+; X64-NEXT:    movw {{.*}}(%rip), %ax
+; X64-NEXT:    xorw %cx, %ax
+; X64-NEXT:    xorl %ecx, %eax
+; X64-NEXT:    movzwl %ax, %eax
+; X64-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
+; X64-NEXT:    addl $-16610, %ecx # imm = 0xBF1E
+; X64-NEXT:    # kill: %CL<def> %CL<kill> %ECX<kill>
+; X64-NEXT:    shrq %cl, %rax
+; X64-NEXT:    movb %al, (%rax)
+; X64-NEXT:    retq
+;
+; 686-LABEL: foo:
+; 686:       # BB#0: # %bb
+; 686-NEXT:    pushl %ebp
+; 686-NEXT:  .Lcfi0:
+; 686-NEXT:    .cfi_def_cfa_offset 8
+; 686-NEXT:  .Lcfi1:
+; 686-NEXT:    .cfi_offset %ebp, -8
+; 686-NEXT:    movl %esp, %ebp
+; 686-NEXT:  .Lcfi2:
+; 686-NEXT:    .cfi_def_cfa_register %ebp
+; 686-NEXT:    andl $-8, %esp
+; 686-NEXT:    subl $8, %esp
+; 686-NEXT:    movzwl var_27, %ecx
+; 686-NEXT:    movw var_22, %ax
+; 686-NEXT:    xorw %cx, %ax
+; 686-NEXT:    xorl %ecx, %eax
+; 686-NEXT:    movzwl %ax, %eax
+; 686-NEXT:    movl %eax, (%esp)
+; 686-NEXT:    movl $0, {{[0-9]+}}(%esp)
+; 686-NEXT:    addl $-16610, %ecx # imm = 0xBF1E
+; 686-NEXT:    xorl %edx, %edx
+; 686-NEXT:    shrdl %cl, %edx, %eax
+; 686-NEXT:    testb $32, %cl
+; 686-NEXT:    jne .LBB0_2
+; 686-NEXT:  # BB#1: # %bb
+; 686-NEXT:    movl %eax, %edx
+; 686-NEXT:  .LBB0_2: # %bb
+; 686-NEXT:    movb %dl, (%eax)
+; 686-NEXT:    movl %ebp, %esp
+; 686-NEXT:    popl %ebp
+; 686-NEXT:    retl
+bb:
+  %tmp = alloca i64, align 8
+  %tmp1 = load i16, i16* @var_22, align 2
+  %tmp2 = zext i16 %tmp1 to i32
+  %tmp3 = load i16, i16* @var_27, align 2
+  %tmp4 = zext i16 %tmp3 to i32
+  %tmp5 = xor i32 %tmp2, %tmp4
+  %tmp6 = load i16, i16* @var_27, align 2
+  %tmp7 = zext i16 %tmp6 to i32
+  %tmp8 = xor i32 %tmp5, %tmp7
+  %tmp9 = sext i32 %tmp8 to i64
+  store i64 %tmp9, i64* %tmp, align 8
+  %tmp10 = load i16, i16* @var_22, align 2
+  %tmp11 = zext i16 %tmp10 to i32
+  %tmp12 = load i16, i16* @var_27, align 2
+  %tmp13 = zext i16 %tmp12 to i32
+  %tmp14 = xor i32 %tmp11, %tmp13
+  %tmp15 = load i16, i16* @var_27, align 2
+  %tmp16 = zext i16 %tmp15 to i32
+  %tmp17 = xor i32 %tmp14, %tmp16
+  %tmp18 = sext i32 %tmp17 to i64
+  %tmp19 = load i16, i16* @var_27, align 2
+  %tmp20 = zext i16 %tmp19 to i32
+  %tmp21 = sub nsw i32 %tmp20, 16610
+  %tmp22 = zext i32 %tmp21 to i64
+  %tmp23 = ashr i64 %tmp18, %tmp22
+  %tmp24 = trunc i64 %tmp23 to i8
+  store i8 %tmp24, i8* undef, align 1
+  ret void
+}