Skip to content

[StatepointLowering] Use FrameIndex instead of TargetFrameIndex #153555

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.

Sign up for GitHub

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

Jump to bottom
Merged
c-rhodes merged 2 commits into from
Aug 15, 2025
Merged

[StatepointLowering] Use FrameIndex instead of TargetFrameIndex #153555

c-rhodes merged 2 commits into from
Aug 15, 2025

Conversation

@c-rhodes
Copy link
Collaborator

@c-rhodes c-rhodes commented Aug 14, 2025

TargetFrameIndex shouldn't be used as an operand to target-independent node such as a load. This causes ISel issues.

#81635 fixed a similar issue with this code using a TargetConstant, instead of a Constant.

Fixes #142314.

@llvmbot
Copy link
Member

llvmbot commented Aug 14, 2025 •
edited
Loading

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Cullen Rhodes (c-rhodes)

Changes

TargetFrameIndex shouldn't be used as an operand to target-independent node such as a load. This causes ISel issues.

#81635 fixed a similar issue with this code using a TargetConstant, instead of a Constant.

Fixes #142314.

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

3 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp (+1-1)
  • (added) llvm/test/CodeGen/AArch64/pr142314.ll (+35)
  • (modified) llvm/test/CodeGen/X86/pr33010.ll (+4-7)
diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
index 80aeefe8e068a..46a5e44374e1c 100644
--- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -1258,7 +1258,7 @@ void SelectionDAGBuilder::visitGCRelocate(const GCRelocateInst &Relocate) {
 
   if (Record.type == RecordType::Spill) {
     unsigned Index = Record.payload.FI;
-    SDValue SpillSlot = DAG.getTargetFrameIndex(Index, getFrameIndexTy());
+    SDValue SpillSlot = DAG.getFrameIndex(Index, getFrameIndexTy());
 
     // All the reloads are independent and are reading memory only modified by
     // statepoints (i.e. no other aliasing stores); informing SelectionDAG of
diff --git a/llvm/test/CodeGen/AArch64/pr142314.ll b/llvm/test/CodeGen/AArch64/pr142314.ll
new file mode 100644
index 0000000000000..1be60eb745424
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/pr142314.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=aarch64 | FileCheck %s
+
+; Crash reproducer for: https://github.com/llvm/llvm-project/issues/142314
+
+define <2 x ptr addrspace(1)> @widget() gc "statepoint-example" {
+; CHECK-LABEL: widget:
+; CHECK:       // %bb.0: // %bb
+; CHECK-NEXT:    sub sp, sp, #32
+; CHECK-NEXT:    str x30, [sp, #16] // 8-byte Folded Spill
+; CHECK-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-NEXT:    .cfi_offset w30, -16
+; CHECK-NEXT:    movi v0.2d, #0000000000000000
+; CHECK-NEXT:    mov x0, xzr
+; CHECK-NEXT:    mov x1, xzr
+; CHECK-NEXT:    str q0, [sp]
+; CHECK-NEXT:    movi d0, #0000000000000000
+; CHECK-NEXT:    blr xzr
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    movi v0.2d, #0000000000000000
+; CHECK-NEXT:    mov x8, sp
+; CHECK-NEXT:    ldr x30, [sp, #16] // 8-byte Folded Reload
+; CHECK-NEXT:    orr x8, x8, #0x8
+; CHECK-NEXT:    ld1 { v0.d }[1], [x8]
+; CHECK-NEXT:    add sp, sp, #32
+; CHECK-NEXT:    ret
+bb:
+  %call31 = call token (i64, i32, ptr, i32, i32, ...) @llvm.experimental.gc.statepoint.p0(i64 0, i32 0, ptr elementtype(i32 (ptr addrspace(1), ptr addrspace(1), double)) null, i32 3, i32 0, ptr addrspace(1) null, ptr addrspace(1) null, double 0.000000e+00, i32 0, i32 0) [ "gc-live"(ptr addrspace(1) null, <2 x ptr addrspace(1)> zeroinitializer, ptr addrspace(1) null) ]
+  %call4 = call coldcc <2 x ptr addrspace(1)> @llvm.experimental.gc.relocate.v2p1(token %call31, i32 0, i32 1) ; (null, zeroinitializer)
+  %shufflevector = shufflevector <2 x ptr addrspace(1)> zeroinitializer, <2 x ptr addrspace(1)> %call4, <2 x i32> <i32 0, i32 3>
+  ret <2 x ptr addrspace(1)> %shufflevector
+}
+
+declare token @llvm.experimental.gc.statepoint.p0(i64 immarg, i32 immarg, ptr, i32 immarg, i32 immarg, ...)
+declare <2 x ptr addrspace(1)> @llvm.experimental.gc.relocate.v2p1(token, i32 immarg, i32 immarg)
diff --git a/llvm/test/CodeGen/X86/pr33010.ll b/llvm/test/CodeGen/X86/pr33010.ll
index 6f0ce664e06d8..41e44dbc59b9b 100644
--- a/llvm/test/CodeGen/X86/pr33010.ll
+++ b/llvm/test/CodeGen/X86/pr33010.ll
@@ -19,13 +19,10 @@ define ptr addrspace(1) @test(ptr addrspace(1) %a, ptr addrspace(1) %b, i1 %whic
 ; CHECK-NEXT:    callq f@PLT
 ; CHECK-NEXT:  .Ltmp0:
 ; CHECK-NEXT:    testb $1, %bl
-; CHECK-NEXT:    je .LBB0_1
-; CHECK-NEXT:  # %bb.2: # %entry
-; CHECK-NEXT:    movq (%rsp), %rax
-; CHECK-NEXT:    jmp .LBB0_3
-; CHECK-NEXT:  .LBB0_1:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rax
-; CHECK-NEXT:  .LBB0_3: # %entry
+; CHECK-NEXT:    movq %rsp, %rax
+; CHECK-NEXT:    leaq {{[0-9]+}}(%rsp), %rcx
+; CHECK-NEXT:    cmovneq %rax, %rcx
+; CHECK-NEXT:    movq (%rcx), %rax
 ; CHECK-NEXT:    addq $16, %rsp
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    popq %rbx

@llvmbot
Copy link
Member

llvmbot commented Aug 14, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Cullen Rhodes (c-rhodes)

Changes

TargetFrameIndex shouldn't be used as an operand to target-independent node such as a load. This causes ISel issues.

#81635 fixed a similar issue with this code using a TargetConstant, instead of a Constant.

Fixes #142314.

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

3 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp (+1-1)
  • (added) llvm/test/CodeGen/AArch64/pr142314.ll (+35)
  • (modified) llvm/test/CodeGen/X86/pr33010.ll (+4-7)
diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
index 80aeefe8e068a..46a5e44374e1c 100644
--- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -1258,7 +1258,7 @@ void SelectionDAGBuilder::visitGCRelocate(const GCRelocateInst &Relocate) {
 
   if (Record.type == RecordType::Spill) {
     unsigned Index = Record.payload.FI;
-    SDValue SpillSlot = DAG.getTargetFrameIndex(Index, getFrameIndexTy());
+    SDValue SpillSlot = DAG.getFrameIndex(Index, getFrameIndexTy());
 
     // All the reloads are independent and are reading memory only modified by
     // statepoints (i.e. no other aliasing stores); informing SelectionDAG of
diff --git a/llvm/test/CodeGen/AArch64/pr142314.ll b/llvm/test/CodeGen/AArch64/pr142314.ll
new file mode 100644
index 0000000000000..1be60eb745424
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/pr142314.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=aarch64 | FileCheck %s
+
+; Crash reproducer for: https://github.com/llvm/llvm-project/issues/142314
+
+define <2 x ptr addrspace(1)> @widget() gc "statepoint-example" {
+; CHECK-LABEL: widget:
+; CHECK:       // %bb.0: // %bb
+; CHECK-NEXT:    sub sp, sp, #32
+; CHECK-NEXT:    str x30, [sp, #16] // 8-byte Folded Spill
+; CHECK-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-NEXT:    .cfi_offset w30, -16
+; CHECK-NEXT:    movi v0.2d, #0000000000000000
+; CHECK-NEXT:    mov x0, xzr
+; CHECK-NEXT:    mov x1, xzr
+; CHECK-NEXT:    str q0, [sp]
+; CHECK-NEXT:    movi d0, #0000000000000000
+; CHECK-NEXT:    blr xzr
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    movi v0.2d, #0000000000000000
+; CHECK-NEXT:    mov x8, sp
+; CHECK-NEXT:    ldr x30, [sp, #16] // 8-byte Folded Reload
+; CHECK-NEXT:    orr x8, x8, #0x8
+; CHECK-NEXT:    ld1 { v0.d }[1], [x8]
+; CHECK-NEXT:    add sp, sp, #32
+; CHECK-NEXT:    ret
+bb:
+  %call31 = call token (i64, i32, ptr, i32, i32, ...) @llvm.experimental.gc.statepoint.p0(i64 0, i32 0, ptr elementtype(i32 (ptr addrspace(1), ptr addrspace(1), double)) null, i32 3, i32 0, ptr addrspace(1) null, ptr addrspace(1) null, double 0.000000e+00, i32 0, i32 0) [ "gc-live"(ptr addrspace(1) null, <2 x ptr addrspace(1)> zeroinitializer, ptr addrspace(1) null) ]
+  %call4 = call coldcc <2 x ptr addrspace(1)> @llvm.experimental.gc.relocate.v2p1(token %call31, i32 0, i32 1) ; (null, zeroinitializer)
+  %shufflevector = shufflevector <2 x ptr addrspace(1)> zeroinitializer, <2 x ptr addrspace(1)> %call4, <2 x i32> <i32 0, i32 3>
+  ret <2 x ptr addrspace(1)> %shufflevector
+}
+
+declare token @llvm.experimental.gc.statepoint.p0(i64 immarg, i32 immarg, ptr, i32 immarg, i32 immarg, ...)
+declare <2 x ptr addrspace(1)> @llvm.experimental.gc.relocate.v2p1(token, i32 immarg, i32 immarg)
diff --git a/llvm/test/CodeGen/X86/pr33010.ll b/llvm/test/CodeGen/X86/pr33010.ll
index 6f0ce664e06d8..41e44dbc59b9b 100644
--- a/llvm/test/CodeGen/X86/pr33010.ll
+++ b/llvm/test/CodeGen/X86/pr33010.ll
@@ -19,13 +19,10 @@ define ptr addrspace(1) @test(ptr addrspace(1) %a, ptr addrspace(1) %b, i1 %whic
 ; CHECK-NEXT:    callq f@PLT
 ; CHECK-NEXT:  .Ltmp0:
 ; CHECK-NEXT:    testb $1, %bl
-; CHECK-NEXT:    je .LBB0_1
-; CHECK-NEXT:  # %bb.2: # %entry
-; CHECK-NEXT:    movq (%rsp), %rax
-; CHECK-NEXT:    jmp .LBB0_3
-; CHECK-NEXT:  .LBB0_1:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rax
-; CHECK-NEXT:  .LBB0_3: # %entry
+; CHECK-NEXT:    movq %rsp, %rax
+; CHECK-NEXT:    leaq {{[0-9]+}}(%rsp), %rcx
+; CHECK-NEXT:    cmovneq %rax, %rcx
+; CHECK-NEXT:    movq (%rcx), %rax
 ; CHECK-NEXT:    addq $16, %rsp
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    popq %rbx

@c-rhodes
Copy link
Collaborator Author

c-rhodes commented Aug 14, 2025

I'm not really familiar with the statepoint lowering but was able to fix the crash following Eli's comment #142314 (comment).

Would appreciate some input from someone with x86 knowledge if the test change is ok, I noticed elsewhere in the statepoint lowering it explicitly uses TargetFrameIndex to avoid selecting LEA:

llvm-project/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

Lines 386 to 387 in ddb2dc5

// We use TargetFrameIndex so that isel will not select it into LEA
Loc = Builder.DAG.getTargetFrameIndex(Index, Builder.getFrameIndexTy());

arsenm
arsenm reviewed Aug 14, 2025
View reviewed changes
preames
preames approved these changes Aug 14, 2025
View reviewed changes
Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

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

Haven't looked at this stuff in years, but I think this is fine. The test change looks to be irrelevant.

@c-rhodes
[StatepointLowering] Use FrameIndex instead of TargetFrameIndex
b8b805a 
TargetFrameIndex shouldn't be used as an operand to target-independent
node such as a load. This causes ISel issues.

\llvm#80294 fixed a similar issue with this code using a TargetConstant,
instead of a Constant.

Fixes llvm#142314.
@c-rhodes
add nounwind
f88455e
@c-rhodes c-rhodes merged commit 393c211 into llvm:main Aug 15, 2025
8 checks passed
@c-rhodes c-rhodes deleted the gh-issue-#142314 branch August 15, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arsenm arsenm arsenm approved these changes

@preames preames preames approved these changes

@efriedma-quic efriedma-quic Awaiting requested review from efriedma-quic

Assignees

No one assigned

Labels

backend:AArch64 backend:X86 llvm:SelectionDAG SelectionDAGISel as well

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

unhandled opcode in isAArch64FrameOffsetLegal

4 participants

@c-rhodes @llvmbot @arsenm @preames