[DAGCombiner] Don't create sexts of deleted xors when they were in-visit replaced

[laytonio]

Summary

Fixes a bug introduced by D91589.

When folding (sext (not i1 x)) -> (add (zext i1 x), -1), we try to replace the not first when possible. If we replace the not in-visit, then the now invalidated node will be returned, and subsequently we will return an invalid sext. In cases where the not is replaced in-visit we can simply return SDValue, as the not in the current sext should have already been replaced.

Thanks @jgorbe, for finding the below reproducer.

The following reduced test case crashes clang when built with clang -O1 -frounding-math:

template <class> class a {
  int b() { return c == 0.0 ? 0 : -1; }
  int c;
};
template class a<long>;

A debug build of clang produces this "assertion failed" error:

clang: /home/jgorbe/code/llvm/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:264: void {anonymous}::DAGCombiner::AddToWorklist(llvm::
SDNode*): Assertion `N->getOpcode() != ISD::DELETED_NODE && "Deleted Node added to Worklist"' failed.

[spatel]

We need a codegen test to verify that the crash is fixed. I used -emit-llvm on the included example to get this (please reduce further if possible):

target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.15.0"

%class.a = type { i32 }

define i32 @_ZN1aIlE1bEv(%class.a* nonnull dereferenceable(4) %this) {
entry:
  %c = getelementptr inbounds %class.a, %class.a* %this, i64 0, i32 0
  %0 = load i32, i32* %c, align 4, !tbaa !3
  %conv = tail call double @llvm.experimental.constrained.sitofp.f64.i32(i32 %0, metadata !"round.dynamic", metadata !"fpexcept.ignore") #2
  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f64(double %conv, double 0.000000e+00, metadata !"oeq", metadata !"fpexcept.ignore") #2
  %not.cmp = xor i1 %cmp, true
  %cond = sext i1 %not.cmp to i32
  ret i32 %cond
}

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10724–10735

Instead of visitXOR() - do we need a visitXORLike op instead here? Or maybe this should be handled by SimplifyDemandedBits?

[laytonio]

Add test case.

[laytonio]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10724–10735

I was planning on trying to extend extractBooleanFlip to include more of the boolean fold and use that here as a follow up.

[spatel]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10724–10735

@RKSimon - are you ok with this extension to solve the crash, or should we change course as suggested (and revert D91589 to solve the crash)?

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10724–10735

As a first step I'm OK with this going in, but I think creating a visitXORLike helper would be better.

[spatel]

LGTM - there's also a code pattern like this in DAGCombiner::rebuildSetCC(), so it's hopefully safe. But as noted, we can probably do better by changing the structure a bit.

[laytonio]

Thanks for the review! Could you please commit this for me? Layton Kifer <[e-mail]>

[rupprecht]

In D93274#2470373, @laytonio wrote:

Thanks for the review! Could you please commit this for me? Layton Kifer <[e-mail]>

Done, and confirmed that the original (unreduced) test case is now building successfully. Thanks!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitSIGN_EXTEND(SDNode *N) {
   }
 
   // fold sext (not i1 X) -> add (zext i1 X), -1
   // TODO: This could be extended to handle bool vectors.
   if (N0.getValueType() == MVT::i1 && isBitwiseNot(N0) && N0.hasOneUse() &&
       (!LegalOperations || (TLI.isOperationLegal(ISD::ZERO_EXTEND, VT) &&
                             TLI.isOperationLegal(ISD::ADD, VT)))) {
     // If we can eliminate the 'not', the sext form should be better
-    if (SDValue NewXor = visitXOR(N0.getNode()))
+    if (SDValue NewXor = visitXOR(N0.getNode())) {
+      // Returning N0 is a form of in-visit replacement that may have
+      // invalidated N0.
+      if (NewXor.getNode() == N0.getNode()) {
+        // Return SDValue here as the xor should have already been replaced in
+        // this sext.
+        return SDValue();
+      } else {
+        // Return a new sext with the new xor.
         return DAG.getNode(ISD::SIGN_EXTEND, DL, VT, NewXor);
+      }
+    }
 
     SDValue Zext = DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0.getOperand(0));
     return DAG.getNode(ISD::ADD, DL, VT, Zext, DAG.getAllOnesConstant(DL, VT));
   }
 
   return SDValue();
 }
 

llvm/test/CodeGen/SystemZ/sext-zext.ll

 ; CHECK-NEXT:    sra %r2, 31
 ; CHECK-NEXT:    br %r14
   %cmp = icmp eq i32 %x, 7
   %xor = xor i1 %cmp, 1
   %sext = sext i1 %xor to i32
   ret i32 %sext
 }
 
+;; fold (sext (not (setcc a, b, cc))) -> (sext (setcc a, b, !cc))
+;; make sure we don't crash if the not gets replaced in-visit
+define i32 @sext_of_not_fsetccs(double %x) {
+; CHECK-LABEL: sext_of_not_fsetccs:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    ltdbr %f0, %f0
+; CHECK-NEXT:    ipm %r0
+; CHECK-NEXT:    afi %r0, 1879048192
+; CHECK-NEXT:    srl %r0, 31
+; CHECK-NEXT:    lcr %r2, %r0
+; CHECK-NEXT:    br %r14
+  %cmp = call i1 @llvm.experimental.constrained.fcmp.f64(double %x, double 0.000000e+00, metadata !"oeq", metadata !"fpexcept.ignore")
+  %xor = xor i1 %cmp, 1
+  %sext = sext i1 %xor to i32
+  ret i32 %sext
+}
+
+declare i1 @llvm.experimental.constrained.fcmp.f64(double, double, metadata, metadata)
+
 ;; TODO: fold (add (zext (setcc a, b, cc)), -1) -> (sext (setcc a, b, !cc))
 define i32 @dec_of_zexted_cmp(i32 %x) {
 ; CHECK-LABEL: dec_of_zexted_cmp:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    chi %r2, 7
 ; CHECK-NEXT:    ipm %r2
 ; CHECK-NEXT:    afi %r2, -268435456
 ; CHECK-NEXT:    srl %r2, 31
 ; CHECK-NEXT:    ahi %r2, -1
 ; CHECK-NEXT:    br %r14
   %cmp = icmp eq i32 %x, 7
   %zext = zext i1 %cmp to i32
   %dec = sub i32 %zext, 1
   ret i32 %dec
 }