Skip to content

Conversation

@dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Feb 21, 2025

N may get merged with existing nodes inside the loop. Early exit when it is deleted to avoid the crash.
Alternative solution: use DAGNodeDeletedListener to refresh the value of N.

Closes #128143.

@dtcxzyw dtcxzyw requested review from RKSimon, bjope and topperc February 21, 2025 10:54
@llvmbot llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Feb 21, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Yingwei Zheng (dtcxzyw)

Changes

N may get merged with existing nodes inside the loop. Early exit when it has been deleted to avoid the crash.
Alternative solution: use DAGNodeDeletedListener to refresh the value of N.

Closes #128143.


Full diff: https://github.com/llvm/llvm-project/pull/128161.diff

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+4-4)
  • (added) llvm/test/CodeGen/X86/pr128143.ll (+32)
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index b07f3814d9d2d..3bd683fcce1e2 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -16158,11 +16158,11 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
       DAG.UpdateNodeOperands(FrozenMaybePoisonOperand.getNode(),
                              MaybePoisonOperand);
     }
-  }
 
-  // This node has been merged with another.
-  if (N->getOpcode() == ISD::DELETED_NODE)
-    return SDValue(N, 0);
+    // This node has been merged with another.
+    if (N->getOpcode() == ISD::DELETED_NODE)
+      return SDValue(N, 0);
+  }
 
   // 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.
diff --git a/llvm/test/CodeGen/X86/pr128143.ll b/llvm/test/CodeGen/X86/pr128143.ll
new file mode 100644
index 0000000000000..2517ad9ebcb6b
--- /dev/null
+++ b/llvm/test/CodeGen/X86/pr128143.ll
@@ -0,0 +1,32 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=x86_64-- | FileCheck %s
+
+@g_1 = external global i8
+@g_2 = external global i8
+
+; Make sure we don't crash on this test.
+
+define i1 @test(i1 %cmp1, i32 %x) {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movq g_2@GOTPCREL(%rip), %rcx
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    cmpq %rcx, g_1@GOTPCREL(%rip)
+; CHECK-NEXT:    setne %al
+; CHECK-NEXT:    cmpl %eax, %esi
+; CHECK-NEXT:    setb %cl
+; CHECK-NEXT:    orb %cl, %al
+; CHECK-NEXT:    andb %dil, %al
+; CHECK-NEXT:    # kill: def $al killed $al killed $eax
+; CHECK-NEXT:    retq
+entry:
+  %cmp2 = icmp ne ptr @g_1, @g_2
+  %fr = freeze ptr @g_1
+  %cmp3 = icmp ne ptr %fr, @g_2
+  %ext1 = zext i1 %cmp3 to i32
+  %sel1 = select i1 %cmp1, i1 %cmp2, i1 false
+  %cmp4 = icmp ult i32 %x, %ext1
+  %sel3 = select i1 %cmp1, i1 %cmp4, i1 false
+  %or = or i1 %sel1, %sel3
+  ret i1 %or
+}

@dtcxzyw dtcxzyw changed the title [DAGCombiner] visitFREEZE: Early exit when N has been deleted [DAGCombiner] visitFREEZE: Early exit when N is deleted Feb 21, 2025
Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one optional minor

@dtcxzyw dtcxzyw merged commit 646e4f2 into llvm:main Feb 22, 2025
11 checks passed
@dtcxzyw dtcxzyw deleted the fix-pr128143 branch February 22, 2025 04:06
@dtcxzyw dtcxzyw added this to the LLVM 20.X Release milestone Feb 22, 2025
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Feb 22, 2025

/cherry-pick 646e4f2

@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2025

/pull-request #128283

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Feb 25, 2025
`N` may get merged with existing nodes inside the loop. Early exit when
it is deleted to avoid the crash.
Alternative solution: use `DAGNodeDeletedListener` to refresh the value
of N.

Closes llvm#128143.

(cherry picked from commit 646e4f2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:X86 llvm:SelectionDAG SelectionDAGISel as well

Projects

Development

Successfully merging this pull request may close these issues.

[DAGCombiner] Assertion `Num < NumOperands && "Invalid child # of SDNode!"' failed.

3 participants