From 84c033d9ba37ef51c0ca6e637ef954ed41d8c32d Mon Sep 17 00:00:00 2001 From: Chuanqi Xu Date: Wed, 7 Jun 2023 10:16:19 +0800 Subject: [PATCH] [LICM] [Coroutines] Don't hoist threadlocals within presplit coroutines 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. Reviewed By: nikic Differential Revision: https://reviews.llvm.org/D151774 --- llvm/lib/Transforms/Scalar/LICM.cpp | 9 +++++++++ llvm/test/Transforms/LICM/sink-with-coroutine.ll | 6 +++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp index 9bfc0b4..5079ef9 100644 --- a/llvm/lib/Transforms/Scalar/LICM.cpp +++ b/llvm/lib/Transforms/Scalar/LICM.cpp @@ -1226,6 +1226,15 @@ bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT, // Handle simple cases by querying alias analysis. MemoryEffects Behavior = AA->getMemoryEffects(CI); + + // FIXME: we don't handle the semantics of thread local well. So that the + // address of thread locals are fake constants in coroutines. So We forbid + // to treat onlyReadsMemory call in coroutines as constants now. Note that + // it is possible to hide a thread local access in a onlyReadsMemory call. + // Remove this check after we handle the semantics of thread locals well. + if (Behavior.onlyReadsMemory() && CI->getFunction()->isPresplitCoroutine()) + return false; + if (Behavior.doesNotAccessMemory()) return true; if (Behavior.onlyReadsMemory()) { diff --git a/llvm/test/Transforms/LICM/sink-with-coroutine.ll b/llvm/test/Transforms/LICM/sink-with-coroutine.ll index f5ce25b..0b3f2e1 100644 --- a/llvm/test/Transforms/LICM/sink-with-coroutine.ll +++ b/llvm/test/Transforms/LICM/sink-with-coroutine.ll @@ -59,20 +59,20 @@ define i64 @hoist_threadlocal() presplitcoroutine { ; CHECK-NEXT: [[P:%.*]] = alloca i64, align 8 ; CHECK-NEXT: br label [[LOOP_PREHEADER:%.*]] ; CHECK: loop.preheader: -; CHECK-NEXT: [[THREAD_LOCAL_0:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @tls) -; CHECK-NEXT: [[THREAD_LOCAL_1:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @tls) -; CHECK-NEXT: [[CMP_0:%.*]] = icmp eq ptr [[THREAD_LOCAL_0]], [[THREAD_LOCAL_1]] ; CHECK-NEXT: br label [[FOR_BODY:%.*]] ; CHECK: for.body: ; CHECK-NEXT: [[I:%.*]] = phi i64 [ 0, [[LOOP_PREHEADER]] ], [ [[I_NEXT:%.*]], [[LOOP_END:%.*]] ] ; CHECK-NEXT: [[I_NEXT]] = add i64 [[I]], 1 +; CHECK-NEXT: [[THREAD_LOCAL_0:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @tls) ; CHECK-NEXT: [[READONLY_0:%.*]] = call ptr @readonly_funcs() ; CHECK-NEXT: [[SUSPEND:%.*]] = call i8 @llvm.coro.suspend(token none, i1 false) ; CHECK-NEXT: switch i8 [[SUSPEND]], label [[EXIT:%.*]] [ ; CHECK-NEXT: i8 0, label [[AWAIT_READY:%.*]] ; CHECK-NEXT: ] ; CHECK: await.ready: +; CHECK-NEXT: [[THREAD_LOCAL_1:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @tls) ; CHECK-NEXT: [[READONLY_1:%.*]] = call ptr @readonly_funcs() +; CHECK-NEXT: [[CMP_0:%.*]] = icmp eq ptr [[THREAD_LOCAL_0]], [[THREAD_LOCAL_1]] ; CHECK-NEXT: [[CMP_1:%.*]] = icmp eq ptr [[READONLY_0]], [[READONLY_1]] ; CHECK-NEXT: [[CMP:%.*]] = and i1 [[CMP_0]], [[CMP_1]] ; CHECK-NEXT: br i1 [[CMP]], label [[NOT_REACHABLE:%.*]], label [[LOOP_END]] -- 2.7.4