[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

1

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

1106

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);
 
+    // New replace instances of N0 and N1
     AddToWorklist(NN0.getNode());
-    if (NN1.getNode())
-      AddToWorklist(NN1.getNode());
 
-    if (Replace0)
+    SDValue RV =
+        DAG.getNode(ISD::TRUNCATE, DL, VT, DAG.getNode(Opc, DL, PVT, NN0, NN1));
+
+    if (Replace0 && N0.getNode() && N0.getOpcode() != ISD::DELETED_NODE &&
+        NN0.getNode() && NN0.getOpcode() != ISD::DELETED_NODE)
       ReplaceLoadWithPromotedLoad(N0.getNode(), NN0.getNode());
-    if (Replace1)
+
+    if (Replace1 && N1.getNode() && N1.getOpcode() != ISD::DELETED_NODE &&
+        NN1.getNode() && NN1.getOpcode() != ISD::DELETED_NODE)
       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));
+    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

+; RUN: llc -O0 -o - %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@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
+
+; Function Attrs: noinline nounwind uwtable
+define void @_Z3foov() #0 {
+entry:
+  store i16 0, i16* @var_825, align 2
+  %0 = load i16, i16* @var_32, align 2
+  %conv = zext i16 %0 to i32
+  %1 = load i16, i16* @var_32, align 2
+  %conv1 = zext i16 %1 to i32
+  %2 = load i16, i16* @var_901, align 2
+  %conv2 = zext i16 %2 to i32
+  %xor = xor i32 %conv1, %conv2
+  %xor3 = xor i32 %conv, %xor
+  %3 = load i16, i16* @var_32, align 2
+  %conv4 = zext i16 %3 to i32
+  %add = add nsw i32 %xor3, %conv4
+  %conv5 = sext i32 %add to i64
+  store i64 %conv5, i64* @var_826, align 8
+  %4 = load i16, i16* @var_32, align 2
+  %conv6 = zext i16 %4 to i64
+  %5 = load i16, i16* @var_32, align 2
+  %conv7 = zext i16 %5 to i64
+  %6 = load i16, i16* @var_901, align 2
+  %conv8 = zext i16 %6 to i32
+  %xor9 = xor i32 51981, %conv8
+  %conv10 = sext i32 %xor9 to i64
+  %xor11 = xor i64 -1142377792914660288, %conv10
+  %xor12 = xor i64 %conv7, %xor11
+  %neg = xor i64 %xor12, -1
+  %xor13 = xor i64 %conv6, %neg
+  %7 = load i16, i16* @var_32, align 2
+  %conv14 = zext i16 %7 to i64
+  %8 = load i16, i16* @var_32, align 2
+  %conv15 = zext i16 %8 to i64
+  %9 = load i16, i16* @var_901, align 2
+  %conv16 = zext i16 %9 to i32
+  %xor17 = xor i32 51981, %conv16
+  %conv18 = sext i32 %xor17 to i64
+  %xor19 = xor i64 -1142377792914660288, %conv18
+  %xor20 = xor i64 %conv15, %xor19
+  %neg21 = xor i64 %xor20, -1
+  %xor22 = xor i64 %conv14, %neg21
+  %10 = load i64, i64* @var_57, align 8
+  %or = or i64 %xor22, %10
+  %or23 = or i64 %xor13, %or
+  %conv24 = trunc i64 %or23 to i16
+  store i16 %conv24, i16* @var_900, align 2
+  %11 = load i64, i64* @var_28, align 8
+  %cmp = icmp ne i64 0, %11
+  %conv25 = zext i1 %cmp to i16
+  store i16 %conv25, i16* @var_827, align 2
+  ret void
+}
+
+attributes #0 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 5.0.0 (trunk 298175) (llvm/trunk 298026)"}

test/CodeGen/X86/pr32345.ll

+; RUN: llc < %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@var_22 = external global i16, align 2
+@var_27 = external global i16, align 2
+
+define void @foo() {
+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
+}