Skip to content

Conversation

@llvmbot
Copy link
Member

@llvmbot llvmbot commented May 20, 2025

Backport c9d6249

Requested by: @MacDue

@llvmbot
Copy link
Member Author

llvmbot commented May 20, 2025

@MacDue What do you think about merging this PR to the release branch?

@llvmbot
Copy link
Member Author

llvmbot commented May 20, 2025

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: None (llvmbot)

Changes

Backport c9d6249

Requested by: @MacDue


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+6-3)
  • (added) llvm/test/CodeGen/X86/pr140491-sincos-lifetimes.ll (+70)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index b416c0efbbc4f..eecfb41c2d319 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -2660,16 +2660,19 @@ bool SelectionDAG::expandMultipleResultFPLibCall(
       continue;
     }
     MachinePointerInfo PtrInfo;
+    SDValue LoadResult =
+        getLoad(Node->getValueType(ResNo), DL, CallChain, ResultPtr, PtrInfo);
+    SDValue OutChain = LoadResult.getValue(1);
+
     if (StoreSDNode *ST = ResultStores[ResNo]) {
       // Replace store with the library call.
-      ReplaceAllUsesOfValueWith(SDValue(ST, 0), CallChain);
+      ReplaceAllUsesOfValueWith(SDValue(ST, 0), OutChain);
       PtrInfo = ST->getPointerInfo();
     } else {
       PtrInfo = MachinePointerInfo::getFixedStack(
           getMachineFunction(), cast<FrameIndexSDNode>(ResultPtr)->getIndex());
     }
-    SDValue LoadResult =
-        getLoad(Node->getValueType(ResNo), DL, CallChain, ResultPtr, PtrInfo);
+
     Results.push_back(LoadResult);
   }
 
diff --git a/llvm/test/CodeGen/X86/pr140491-sincos-lifetimes.ll b/llvm/test/CodeGen/X86/pr140491-sincos-lifetimes.ll
new file mode 100644
index 0000000000000..2ca99bdc4b316
--- /dev/null
+++ b/llvm/test/CodeGen/X86/pr140491-sincos-lifetimes.ll
@@ -0,0 +1,70 @@
+; RUN: llc < %s | FileCheck %s
+
+; This test is reduced from https://github.com/llvm/llvm-project/issues/140491.
+; It checks that when `@llvm.sincos.f32` is expanded to a call to
+; `sincosf(float, float* out_sin, float* out_cos)` and the store of `%cos` to
+; `%computed` is folded into the `sincosf` call. The use of `%cos`in the later
+; `fneg %cos` -- which expands to a load of `%computed`, will perform the load
+; before the `@llvm.lifetime.end.p0(%computed)` to ensure the correct value is
+; taken for `%cos`.
+
+target triple = "x86_64-sie-ps5"
+
+declare void @use_ptr(ptr readonly)
+
+define i32 @sincos_stack_slot_with_lifetime(float %in)  {
+; CHECK-LABEL: sincos_stack_slot_with_lifetime:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushq %rbx
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    subq $32, %rsp
+; CHECK-NEXT:    .cfi_def_cfa_offset 48
+; CHECK-NEXT:    .cfi_offset %rbx, -16
+; CHECK-NEXT:    leaq 12(%rsp), %rdi
+; CHECK-NEXT:    leaq 8(%rsp), %rbx
+; CHECK-NEXT:    movq %rbx, %rsi
+; CHECK-NEXT:    callq sincosf@PLT
+; CHECK-NEXT:    movss 8(%rsp), %xmm0 # xmm0 = mem[0],zero,zero,zero
+; CHECK-NEXT:    movaps %xmm0, 16(%rsp) # 16-byte Spill
+; CHECK-NEXT:    movq %rbx, %rdi
+; CHECK-NEXT:    callq use_ptr
+; CHECK-NEXT:    movss 12(%rsp), %xmm0 # xmm0 = mem[0],zero,zero,zero
+; CHECK-NEXT:    xorps {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
+; CHECK-NEXT:    movss %xmm0, 8(%rsp)
+; CHECK-NEXT:    leaq 8(%rsp), %rdi
+; CHECK-NEXT:    callq use_ptr
+; CHECK-NEXT:    movaps 16(%rsp), %xmm0 # 16-byte Reload
+; CHECK-NEXT:    xorps {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
+; CHECK-NEXT:    movss %xmm0, 8(%rsp)
+; CHECK-NEXT:    leaq 8(%rsp), %rdi
+; CHECK-NEXT:    callq use_ptr
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    addq $32, %rsp
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    popq %rbx
+; CHECK-NEXT:    .cfi_def_cfa_offset 8
+; CHECK-NEXT:    retq
+entry:
+  %computed = alloca float, align 4
+  %computed1 = alloca float, align 4
+  %computed3 = alloca float, align 4
+  %sincos = tail call { float, float } @llvm.sincos.f32(float %in)
+  %sin = extractvalue { float, float } %sincos, 0
+  %cos = extractvalue { float, float } %sincos, 1
+  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %computed)
+  store float %cos, ptr %computed, align 4
+  call void @use_ptr(ptr nonnull %computed)
+  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %computed)
+  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %computed1)
+  %fneg_sin = fneg float %sin
+  store float %fneg_sin, ptr %computed1, align 4
+  call void @use_ptr(ptr nonnull %computed1)
+  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %computed1)
+  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %computed3)
+  %fneg_cos = fneg float %cos
+  store float %fneg_cos, ptr %computed3, align 4
+  call void @use_ptr(ptr nonnull %computed3)
+  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %computed3)
+  ret i32 0
+}
+

@MacDue
Copy link
Member

MacDue commented May 20, 2025

Not sure why the bot is asking me (I think it's fine, but I requested the backport).

cc @arsenm, @RKSimon

@MacDue MacDue requested review from RKSimon and arsenm May 20, 2025 10:06
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

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status May 20, 2025
…lvm#140525)

The load not being included in the chain meant that it could materialize
after a `@llvm.lifetime.end` annotation on the pointer. This could
result in miscompiles if the stack slot is reused for another value.

Fixes llvm#140491

(cherry picked from commit c9d6249)
@tstellar tstellar merged commit 9b08325 into llvm:release/20.x May 24, 2025
8 of 10 checks passed
@github-project-automation github-project-automation bot moved this from Needs Merge to Done in LLVM Release Status May 24, 2025
@github-actions
Copy link

@MacDue (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

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.

5 participants