[Clang][CodeGen] Do not emit lifetime intrinsics for coro promise alloca

[NewSigma]

Coro promise has same lifetime as coro frame. It do not need explicit lifetime guarding. If we add lifetimes to it, middle end passes may assume promise dead after lifetime.end, leading to mis-optimizations.

Fix #120200

[llvmbot]

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-clang

Author: Weibo He (NewSigma)

Changes

Coro promise has same lifetime as coro frame. It do not need explicit lifetime guarding. If we add lifetimes to it, middle end passes may assume promise dead after lifetime.end, leading to mis-optimizations.

Fix #120200

---

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

3 Files Affected:

• (modified) clang/lib/CodeGen/CGDecl.cpp (+5)
• (modified) clang/test/CodeGenCoroutines/coro-params.cpp (-3)
• (modified) clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp (+1-1)

diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a8f7f6a42ecb..556adebc6fa39 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1343,6 +1343,11 @@ llvm::Value *CodeGenFunction::EmitLifetimeStart(llvm::TypeSize Size,
   if (!ShouldEmitLifetimeMarkers)
     return nullptr;
 
+  // No lifetimes on promise alloca, or middle end passes will assume promise
+  // dead after lifetime.end, leading to mis-optimization
+  if (Addr->getName() == "__promise")
+    return nullptr;
+
   assert(Addr->getType()->getPointerAddressSpace() ==
              CGM.getDataLayout().getAllocaAddrSpace() &&
          "Pointer should be in alloca address space");
diff --git a/clang/test/CodeGenCoroutines/coro-params.cpp b/clang/test/CodeGenCoroutines/coro-params.cpp
index 719726cca29c5..4f13e093197ff 100644
--- a/clang/test/CodeGenCoroutines/coro-params.cpp
+++ b/clang/test/CodeGenCoroutines/coro-params.cpp
@@ -84,7 +84,6 @@ void f(int val, MoveOnly moParam, MoveAndCopy mcParam, TrivialABI trivialParam)
   // CHECK-NEXT: call void @_ZN11MoveAndCopyC1EOS_(ptr {{[^,]*}} %[[McCopy]], ptr noundef nonnull align 4 dereferenceable(4) %[[McParam]]) #
   // CHECK-NEXT: call void @llvm.lifetime.start.p0(
   // CHECK-NEXT: call void @_ZN10TrivialABIC1EOS_(ptr {{[^,]*}} %[[TrivialCopy]], ptr {{[^,]*}} %[[TrivialAlloca]])
-  // CHECK-NEXT: call void @llvm.lifetime.start.p0(
   // CHECK-NEXT: invoke void @_ZNSt16coroutine_traitsIJvi8MoveOnly11MoveAndCopy10TrivialABIEE12promise_typeC1Ev(
 
   // CHECK: call void @_ZN14suspend_always12await_resumeEv(
@@ -106,7 +105,6 @@ void f(int val, MoveOnly moParam, MoveAndCopy mcParam, TrivialABI trivialParam)
 
   // Destroy promise, then parameter copies:
   // CHECK: call void @_ZNSt16coroutine_traitsIJvi8MoveOnly11MoveAndCopy10TrivialABIEE12promise_typeD1Ev(ptr {{[^,]*}} %__promise)
-  // CHECK-NEXT: call void @llvm.lifetime.end.p0(
   // CHECK-NEXT: call void @_ZN10TrivialABID1Ev(ptr {{[^,]*}} %[[TrivialCopy]])
   // CHECK-NEXT: call void @llvm.lifetime.end.p0(
   // CHECK-NEXT: call void @_ZN11MoveAndCopyD1Ev(ptr {{[^,]*}} %[[McCopy]])
@@ -135,7 +133,6 @@ void dependent_params(T x, U, U y) {
   // CHECK-NEXT: call void @_ZN1BC1EOS_(ptr {{[^,]*}} %[[unnamed_copy]], ptr noundef nonnull align 4 dereferenceable(512) %0)
   // CHECK-NEXT: call void @llvm.lifetime.start.p0(
   // CHECK-NEXT: call void @_ZN1BC1EOS_(ptr {{[^,]*}} %[[y_copy]], ptr noundef nonnull align 4 dereferenceable(512) %y)
-  // CHECK-NEXT: call void @llvm.lifetime.start.p0(
   // CHECK-NEXT: invoke void @_ZNSt16coroutine_traitsIJv1A1BS1_EE12promise_typeC1Ev(
 
   co_return;
diff --git a/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp b/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp
index d71c2c558996a..d028c127e1e21 100644
--- a/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp
+++ b/clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp
@@ -52,7 +52,7 @@ coroutine ArrayInitCoro() {
     // CHECK-NEXT:  store ptr %arrayinit.element, ptr %arrayinit.endOfInit.reload.addr, align 8
     co_await Awaiter{}
     // CHECK-NEXT:  @_ZNSt14suspend_always11await_readyEv
-    // CHECK-NEXT:  br i1 %{{.+}}, label %await.ready, label %CoroSave30
+    // CHECK-NEXT:  br i1 %{{.+}}, label %await.ready, label %CoroSave29
   };
   // CHECK:       await.cleanup:                                    ; preds = %AfterCoroSuspend{{.*}}
   // CHECK-NEXT:    br label %cleanup{{.*}}.from.await.cleanup

[NewSigma]

Here is a example

define i32 @fn() {
entry:
  %__promise = alloca i32, align 4
  %id = call token @llvm.coro.id(i32 16, ptr nonnull %__promise, ptr nonnull @fn, ptr null)
  %hdl = call ptr @llvm.coro.begin(token %id, ptr null) #14
  %promise.addr = call ptr @llvm.coro.promise(ptr %hdl, i32 4, i1 false) #14
  call void @llvm.lifetime.start.p0(i64 4, ptr %promise.addr) #2
  store i32 5, ptr %promise.addr, align 4 ; DSE eliminates
  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %promise.addr) #2
  %0 = call i1 @llvm.coro.end(ptr null, i1 false, token none) #14
  %value = load i32, ptr %promise.addr, align 4
  ret i32 %value
}

[ChuanqiXu9]

We should avoid comparing string as much as possible. And also it is not good to insert coroutines logic to CGDecl.cpp. We'd either doing this in CGCoroutine or CoroEarly.

[zmodem]

If we add lifetimes to it, middle end passes may assume promise dead after lifetime.end, leading to mis-optimizations.

It sounds like the issue is that we emit incorrect lifetime intrinsics for it, not the lifetime intrinsics itself.

Here is a example

Was that generated by Clang?

It would be good to have a small C++ example showing the incorrect lifetime intrinsics, that could also be used as a test.

[NewSigma]

After further consideration, I think this is not a satisfactory solution to the issue. I may revisit it if I develop a concrete plan. Apologies for any inconvenience.