Skip to content

[DAGCombiner] visitFREEZE: Early exit when N is deleted #128161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 22, 2025

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, topperc and bjope 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