[PATCH] D151774: [LICM] Don't hoist threadlocals within presplit coroutines

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: nikic, jdoerfert, jyknight, nlopes, nhaehnle.
Herald added subscribers: StephenFan, asbirlea, hiraditya.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Close https://github.com/llvm/llvm-project/issues/63022

This is the following of https://reviews.llvm.org/D135550, which is discussed in https://discourse.llvm.org/t/rfc-unify-memory-effect-attributes/65579. In my imagination, we could fix the issue fundamentally after we introduces new memory kind `thread id`. But I am not very sure if we can fix the issue fundamentally in time.

Besides that, I think the correctness is the most important. So it should not be bad to land this given it is innocent.


https://reviews.llvm.org/D151774

Files:
  llvm/lib/Transforms/Scalar/LICM.cpp
  llvm/test/Transforms/LICM/sink-with-coroutine.ll


Index: llvm/test/Transforms/LICM/sink-with-coroutine.ll
===================================================================
--- llvm/test/Transforms/LICM/sink-with-coroutine.ll
+++ llvm/test/Transforms/LICM/sink-with-coroutine.ll
@@ -48,5 +48,56 @@
   ret i64 0
 }
 
+ at tls = thread_local global i32 0
+
+define i64 @hoist_threadlocal() presplitcoroutine {
+; CHECK-LABEL: @hoist_threadlocal
+; CHECK: loop.preheader:
+; CHECK-NEXT: br label %for.body
+; CHECK: for.body:
+; CHECK-NEXT: %i = phi i64
+; CHECK-NEXT: %i.next = add i64 %i
+; CHECK-NEXT: %0 = call ptr @llvm.threadlocal.address
+; CHECK-NEXT: %suspend = call i8 @llvm.coro.suspend
+; CHECK-NEXT: switch i8 %suspend, label %exit 
+; CHECK-NEXT: i8 0, label %await.ready 
+; CHECK: await.ready:
+; CHECK-NEXT: %1 = call ptr @llvm.threadlocal.address
+entry:
+  %p = alloca i64
+  br label %loop.preheader
+
+loop.preheader:
+  br label %for.body
+
+for.body:
+  %i = phi i64 [ 0, %loop.preheader ], [ %i.next, %loop.end ]
+  %i.next = add i64 %i, 1
+  %0 = call ptr @llvm.threadlocal.address(ptr @tls)
+  %suspend = call i8 @llvm.coro.suspend(token none, i1 false)
+    switch i8 %suspend, label %exit [
+    i8 0, label %await.ready
+  ]
+
+await.ready:
+  %1 = call ptr @llvm.threadlocal.address(ptr @tls)
+  %cmp = icmp eq ptr %0, %1
+  br i1 %cmp, label %not.reachable, label %loop.end
+
+not.reachable:
+  call void @not.reachable()
+  br label %loop.end
+
+loop.end:
+  %loop.end.cond = icmp ugt i64 %i.next, 5
+  br i1 %cmp, label %exit, label %for.body
+
+exit:
+  %res = call i1 @llvm.coro.end(ptr null, i1 false)
+  ret i64 0
+}
+
 declare i8  @llvm.coro.suspend(token, i1)
 declare i1  @llvm.coro.end(ptr, i1)
+declare nonnull ptr @llvm.threadlocal.address(ptr nonnull) nounwind readnone willreturn
+declare void @not.reachable()
Index: llvm/lib/Transforms/Scalar/LICM.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/LICM.cpp
+++ llvm/lib/Transforms/Scalar/LICM.cpp
@@ -1219,6 +1219,14 @@
     if (CI->isConvergent())
       return false;
 
+    // FIXME: We don't handle the semantics of thread local well. So that the
+    // address thread locals are fake constants in coroutines. We forbid this
+    // explcitly now.
+    // Remove this check after we handle the semantics of thread locals well.
+    if (CI->getIntrinsicID() == Intrinsic::threadlocal_address &&
+        CI->getFunction()->isPresplitCoroutine())
+      return false;
+
     using namespace PatternMatch;
     if (match(CI, m_Intrinsic<Intrinsic::assume>()))
       // Assumes don't actually alias anything or throw


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151774.526884.patch
Type: text/x-patch
Size: 2614 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230531/6ca1c43b/attachment.bin>