[DAGCombine]Don't check for Undef/Poison if the node is deleted

[Naville]

Summary

Fixed https://github.com/llvm/llvm-project/issues/59891

Please also commit for me since I dont have commit access

[RKSimon]

Please can you add a test case from the issue

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

14405

(style) early out instead of a if-else chain

if(N0.getOpcode() == ISD::DELETED_NODE)
  return SDValue()
// NOTE: this strips poison generating flags.
SDValue R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);

[Naville]

Add test case, fix style issue

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

14405

Can you confirm that the DELETED_NODE is appearing due to the UpdateNodeOperands call above (sorry you didn't generate the diff with context so I can't point to it)

I really need to step through the debugger myself, but can't right now :(

[Naville]

Can confirm no DELETED_NODE exists before the entire DAG processing logic,
DAG looks like this before ` for (SDValue MaybePoisonOperand : MaybePoisonOperands) `:

SelectionDAG has 12 nodes:
  t0: ch,glue = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %0
            t3: i64 = freeze t2
          t4: i1 = truncate t3
          t5: i1 = truncate t2
        t6: i1 = xor t4, t5
      t7: i1 = freeze t6
    t9: i32 = zero_extend t7
  t11: ch,glue = CopyToReg t0, Register:i32 $w0, t9
  t12: ch = AArch64ISD::RET_FLAG t11, Register:i32 $w0, t11:1

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

14405

OK - in which case this can be put straight after the UpdateNodeOperands:

for (SDValue MaybePoisonOperand : MaybePoisonOperands) {
  // Don't replace every single UNDEF everywhere with frozen UNDEF, though.
  if (MaybePoisonOperand.getOpcode() == ISD::UNDEF)
    continue;
  // First, freeze each offending operand.
  SDValue FrozenMaybePoisonOperand = DAG.getFreeze(MaybePoisonOperand);
  // Then, change all other uses of unfrozen operand to use frozen operand.
  DAG.ReplaceAllUsesOfValueWith(MaybePoisonOperand, FrozenMaybePoisonOperand);
  if (FrozenMaybePoisonOperand.getOpcode() == ISD::FREEZE &&
      FrozenMaybePoisonOperand.getOperand(0) == FrozenMaybePoisonOperand) {
    // But, that also updated the use in the freeze we just created, thus
    // creating a cycle in a DAG. Let's undo that by mutating the freeze.
    DAG.UpdateNodeOperands(FrozenMaybePoisonOperand.getNode(),
                           MaybePoisonOperand);
  }
}

// Early-out if the node has already been updated in place.
if (N0.getOpcode() == ISD::DELETED_NODE)
  return SDValue();

// Finally, recreate the node, it's operands were updated to use
// frozen operands, so we just need to use it's "original" operands.
SmallVector<SDValue> Ops(N0->op_begin(), N0->op_end());
// Special-handle ISD::UNDEF, each single one of them can be it's own thing.
for (SDValue &Op : Ops) {
  if (Op.getOpcode() == ISD::UNDEF)
    Op = DAG.getFreeze(Op);
}
// NOTE: this strips poison generating flags.
SDValue R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) &&
       "Can't create node that may be undef/poison!");
return R;

llvm/test/CodeGen/AArch64/dagcombine-deleted-freeze.ll

4 ↗

(On Diff #487386)

drop the target/datalayout - and let the -mtriple in the RUN handle it

[Naville]

@RKSimon Please check

[Naville]

Fix broken test case

[lebedev.ri]

This isn't right, we'll no longer be sure that we dropped flags.
I think something like this should be closer to the truth:

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 78cbfe1356f7..a87415c3f5ed 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -14289,6 +14289,7 @@ SDValue DAGCombiner::visitBUILD_PAIR(SDNode *N) {
 
 SDValue DAGCombiner::visitFREEZE(SDNode *N) {
   SDValue N0 = N->getOperand(0);
+  HandleSDNode N0Handle(N0);
 
   if (DAG.isGuaranteedNotToBeUndefOrPoison(N0, /*PoisonOnly*/ false))
     return N0;

[lebedev.ri]

Or better yet,

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 78cbfe1356f7..82fdbf1cc790 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -14341,6 +14341,9 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
                              MaybePoisonOperand);
     }
   }
+  // The whole node may have been updated, so the value we were holding
+  // may no longer be valid. Re-fetch the operand we're `freeze`ing.
+  N0 = N->getOperand(0);
 
   // Finally, recreate the node, it's operands were updated to use
   // frozen operands, so we just need to use it's "original" operands.

[Naville]

Address review issues

[Naville]

@lebedev.ri please review

[lebedev.ri]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

14405

@RKSimon if you are sure about just bailing in case of in-place update,
please feel free to land that version of this patch.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (FrozenMaybePoisonOperand.getOpcode() == ISD::FREEZE &&
         FrozenMaybePoisonOperand.getOperand(0) == FrozenMaybePoisonOperand) {
       // But, that also updated the use in the freeze we just created, thus
       // creating a cycle in a DAG. Let's undo that by mutating the freeze.
       DAG.UpdateNodeOperands(FrozenMaybePoisonOperand.getNode(),
                              MaybePoisonOperand);
     }
   }
 
+  // The whole node may have been updated, so the value we were holding
+  // may no longer be valid. Re-fetch the operand we're `freeze`ing.
+  N0 = N->getOperand(0);
+
   // Finally, recreate the node, it's operands were updated to use
   // frozen operands, so we just need to use it's "original" operands.
   SmallVector<SDValue> Ops(N0->op_begin(), N0->op_end());
   // Special-handle ISD::UNDEF, each single one of them can be it's own thing.
   for (SDValue &Op : Ops) {
     if (Op.getOpcode() == ISD::UNDEF)
       Op = DAG.getFreeze(Op);
   }
   // NOTE: this strips poison generating flags.
   SDValue R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
   assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) &&
          "Can't create node that may be undef/poison!");
   return R;
 }
 
 /// We know that BV is a build_vector node with Constant, ConstantFP or Undef
 /// operands. DstEltVT indicates the destination element value type.
 SDValue DAGCombiner::
 ConstantFoldBITCASTofBUILD_VECTOR(SDNode *BV, EVT DstEltVT) {
   EVT SrcEltVT = BV->getValueType(0).getVectorElementType();
 
   // If this is already the right type, we're done.
   if (SrcEltVT == DstEltVT) return SDValue(BV, 0);

llvm/test/CodeGen/AArch64/pr59891-dagcombine-deleted-freeze.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64 | FileCheck %s
+
+; Freezing operand of an operation that is being freezed
+; may invalidate the whole SDValue we were trying to originally freeze.
+define i8 @pr59891(i64 %0) {
+; CHECK-LABEL: pr59891:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov w0, wzr
+; CHECK-NEXT:    ret
+  %2 = freeze i64 %0
+  %3 = trunc i64 %2 to i1
+  %4 = trunc i64 %0 to i1
+  %5 = xor i1 %3, %4
+  %6 = freeze i1 %5
+  %7 = zext i1 %6 to i8
+  ret i8 %7
+}