[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

1096

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

[RKSimon]

test/CodeGen/X86/pr32345.ll

1 ↗

(On Diff #92635)

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

1096

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

1098

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

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (TLI.IsDesirableToPromoteOp(Op, PVT)) {
     assert(PVT != VT && "Don't know what type to promote to!");
 
     bool Replace0 = false;
     SDValue N0 = Op.getOperand(0);
     SDValue NN0 = PromoteOperand(N0, PVT, Replace0);
     if (!NN0.getNode())
       return SDValue();
 
+    AddToWorklist(NN0.getNode());
+    if (Replace0)
+      ReplaceLoadWithPromotedLoad(N0.getNode(), NN0.getNode());
+
     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);
 
-    AddToWorklist(NN0.getNode());
-    if (NN1.getNode())
-      AddToWorklist(NN1.getNode());
-
-    if (Replace0)
-      ReplaceLoadWithPromotedLoad(N0.getNode(), NN0.getNode());
     if (Replace1)
       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));

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)"}