From fa9149a8b06665ddd439b4d4b7ca4af3faaef8e6 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Fri, 9 May 2025 18:43:07 +0800 Subject: [PATCH 01/10] [Coro] Hide promise alloca for later passes --- llvm/docs/Coroutines.rst | 9 ++-- .../llvm/Transforms/Coroutines/CoroShape.h | 11 ++-- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 53 ++++++++++++++++--- llvm/lib/Transforms/Coroutines/Coroutines.cpp | 14 ++++- .../AddressSanitizer/skip-coro.ll | 5 +- llvm/test/Transforms/Coroutines/gh105595.ll | 32 +++++++++++ 6 files changed, 106 insertions(+), 18 deletions(-) create mode 100644 llvm/test/Transforms/Coroutines/gh105595.ll diff --git a/llvm/docs/Coroutines.rst b/llvm/docs/Coroutines.rst index 60e32dc467d27..157b20b7d8d73 100644 --- a/llvm/docs/Coroutines.rst +++ b/llvm/docs/Coroutines.rst @@ -2121,10 +2121,11 @@ Coroutine Transformation Passes =============================== CoroEarly --------- -The pass CoroEarly lowers coroutine intrinsics that hide the details of the -structure of the coroutine frame, but, otherwise not needed to be preserved to -help later coroutine passes. This pass lowers `coro.frame`_, `coro.done`_, -and `coro.promise`_ intrinsics. +The CoroEarly pass ensures later middle end passes correctly interpret coroutine +semantics and lowers coroutine intrinsics that not needed to be preserved to +help later coroutine passes. This pass lowers `coro.promise`_ that outside the +coroutine body, `coro.frame`_ and `coro.done`_ intrinsics. It replace uses of +promise alloca with `coro.promise`_ intrinsic. .. _CoroSplit: diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h index ea93ced1ce29e..aee918946c70f 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h @@ -79,7 +79,8 @@ struct Shape { // Scan the function and collect the above intrinsics for later processing void analyze(Function &F, SmallVectorImpl &CoroFrames, - SmallVectorImpl &UnusedCoroSaves); + SmallVectorImpl &UnusedCoroSaves, + SmallVectorImpl &CoroPromises); // If for some reason, we were not able to find coro.begin, bailout. void invalidateCoroutine(Function &F, SmallVectorImpl &CoroFrames); @@ -87,7 +88,8 @@ struct Shape { void initABI(); // Remove orphaned and unnecessary intrinsics void cleanCoroutine(SmallVectorImpl &CoroFrames, - SmallVectorImpl &UnusedCoroSaves); + SmallVectorImpl &UnusedCoroSaves, + SmallVectorImpl &CoroPromises); // Field indexes for special fields in the switch lowering. struct SwitchFieldIndex { @@ -265,13 +267,14 @@ struct Shape { explicit Shape(Function &F) { SmallVector CoroFrames; SmallVector UnusedCoroSaves; + SmallVector CoroPromises; - analyze(F, CoroFrames, UnusedCoroSaves); + analyze(F, CoroFrames, UnusedCoroSaves, CoroPromises); if (!CoroBegin) { invalidateCoroutine(F, CoroFrames); return; } - cleanCoroutine(CoroFrames, UnusedCoroSaves); + cleanCoroutine(CoroFrames, UnusedCoroSaves, CoroPromises); } }; diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index 5375448d2d2e2..ff1e1c8cc56cf 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -9,6 +9,7 @@ #include "llvm/Transforms/Coroutines/CoroEarly.h" #include "CoroInternal.h" #include "llvm/IR/DIBuilder.h" +#include "llvm/IR/Dominators.h" #include "llvm/IR/Function.h" #include "llvm/IR/IRBuilder.h" #include "llvm/IR/InstIterator.h" @@ -165,6 +166,7 @@ static void setCannotDuplicate(CoroIdInst *CoroId) { void Lowerer::lowerEarlyIntrinsics(Function &F) { CoroIdInst *CoroId = nullptr; + CoroBeginInst *CoroBegin = nullptr; SmallVector CoroFrees; bool HasCoroSuspend = false; for (Instruction &I : llvm::make_early_inc_range(instructions(F))) { @@ -175,6 +177,23 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { switch (CB->getIntrinsicID()) { default: continue; + case Intrinsic::coro_begin: + case Intrinsic::coro_begin_custom_abi: { + auto CBI = cast(&I); + + // Ignore coro id's that aren't pre-split. + auto Id = dyn_cast(CBI->getId()); + if (Id && !Id->getInfo().isPreSplit()) + break; + + if (CoroBegin) + report_fatal_error( + "coroutine should have exactly one defining @llvm.coro.begin"); + CBI->addRetAttr(Attribute::NonNull); + CBI->addRetAttr(Attribute::NoAlias); + CoroBegin = CBI; + break; + } case Intrinsic::coro_free: CoroFrees.push_back(cast(&I)); break; @@ -218,22 +237,44 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { case Intrinsic::coro_destroy: lowerResumeOrDestroy(*CB, CoroSubFnInst::DestroyIndex); break; - case Intrinsic::coro_promise: - lowerCoroPromise(cast(&I)); + case Intrinsic::coro_promise: { + bool OutsideCoro = CoroBegin == nullptr; + if (OutsideCoro) + lowerCoroPromise(cast(&I)); break; + } case Intrinsic::coro_done: lowerCoroDone(cast(&I)); break; } } - // Make sure that all CoroFree reference the coro.id intrinsic. - // Token type is not exposed through coroutine C/C++ builtins to plain C, so - // we allow specifying none and fixing it up here. - if (CoroId) + if (CoroId) { + // Make sure that all CoroFree reference the coro.id intrinsic. + // Token type is not exposed through coroutine C/C++ builtins to plain C, so + // we allow specifying none and fixing it up here. for (CoroFreeInst *CF : CoroFrees) CF->setArgOperand(0, CoroId); + if (auto *PA = CoroId->getPromise()) { + assert(CoroBegin && "Use Switch-Resumed ABI but missing coro.begin"); + + Builder.SetInsertPoint(*CoroBegin->getInsertionPointAfterDef()); + + auto *Alignment = Builder.getInt32(PA->getAlign().value()); + auto *FromPromise = Builder.getInt1(false); + SmallVector Arg{CoroBegin, Alignment, FromPromise}; + auto *PI = + Builder.CreateIntrinsic(Builder.getPtrTy(), Intrinsic::coro_promise, + Arg, {}, "promise.addr"); + PA->replaceUsesWithIf(PI, [CoroId](Use &U) { + bool IsBitcast = U == U.getUser()->stripPointerCasts(); + bool IsCoroId = U.getUser() == CoroId; + return !IsBitcast && !IsCoroId; + }); + } + } + // Coroutine suspention could potentially lead to any argument modified // outside of the function, hence arguments should not have noalias // attributes. diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index 7b59c39283ded..325ad36bc666a 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -192,7 +192,8 @@ static CoroSaveInst *createCoroSave(CoroBeginInst *CoroBegin, // Collect "interesting" coroutine intrinsics. void coro::Shape::analyze(Function &F, SmallVectorImpl &CoroFrames, - SmallVectorImpl &UnusedCoroSaves) { + SmallVectorImpl &UnusedCoroSaves, + SmallVectorImpl &CoroPromises) { clear(); bool HasFinalSuspend = false; @@ -286,6 +287,9 @@ void coro::Shape::analyze(Function &F, } } break; + case Intrinsic::coro_promise: + CoroPromises.push_back(cast(II)); + break; } } } @@ -477,7 +481,8 @@ void coro::AnyRetconABI::init() { void coro::Shape::cleanCoroutine( SmallVectorImpl &CoroFrames, - SmallVectorImpl &UnusedCoroSaves) { + SmallVectorImpl &UnusedCoroSaves, + SmallVectorImpl &CoroPromises) { // The coro.frame intrinsic is always lowered to the result of coro.begin. for (CoroFrameInst *CF : CoroFrames) { CF->replaceAllUsesWith(CoroBegin); @@ -489,6 +494,11 @@ void coro::Shape::cleanCoroutine( for (CoroSaveInst *CoroSave : UnusedCoroSaves) CoroSave->eraseFromParent(); UnusedCoroSaves.clear(); + + for (auto *PI : CoroPromises) { + PI->replaceAllUsesWith(getPromiseAlloca()); + PI->eraseFromParent(); + } } static void propagateCallAttrsFromCallee(CallInst *Call, Function *Callee) { diff --git a/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll index 65b27ee2241dd..ed3af686d5257 100644 --- a/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll +++ b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll @@ -15,8 +15,9 @@ define ptr @foo() #0 { entry: %__promise = alloca %struct.Promise, align 8 %0 = call token @llvm.coro.id(i32 16, ptr nonnull %__promise, ptr null, ptr null) - %1 = call ptr @llvm.coro.noop() - ret ptr %1 + %1 = call ptr @llvm.coro.begin(token %0, ptr null) + %2 = call ptr @llvm.coro.noop() + ret ptr %2 } declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr) diff --git a/llvm/test/Transforms/Coroutines/gh105595.ll b/llvm/test/Transforms/Coroutines/gh105595.ll new file mode 100644 index 0000000000000..cc23419bf7ff4 --- /dev/null +++ b/llvm/test/Transforms/Coroutines/gh105595.ll @@ -0,0 +1,32 @@ +; Test that store-load operation that crosses suspension point will not be eliminated by DSE +; Coro result conversion function that attempts to modify promise shall produce this pattern +; RUN: opt < %s -passes='coro-early,dse' -S | FileCheck %s + +define void @fn() presplitcoroutine { + %__promise = alloca ptr, align 8 + %id = call token @llvm.coro.id(i32 16, ptr %__promise, ptr @fn, ptr null) + %hdl = call ptr @llvm.coro.begin(token %id, ptr null) +; CHECK: %promise.addr = call ptr @llvm.coro.promise(ptr %hdl, i32 8, i1 false) + %save = call token @llvm.coro.save(ptr null) + %sp = call i8 @llvm.coro.suspend(token %save, i1 false) + %flag = icmp ule i8 %sp, 1 + br i1 %flag, label %resume, label %suspend + +resume: +; CHECK: call void @use_value(ptr %promise.addr) + call void @use_value(ptr %__promise) + br label %suspend + +suspend: + call i1 @llvm.coro.end(ptr null, i1 false, token none) +; load value when resume +; CHECK: %null = load ptr, ptr %promise.addr, align 8 + %null = load ptr, ptr %__promise, align 8 + call void @use_value(ptr %null) +; store value when suspend +; CHECK: store ptr null, ptr %promise.addr, align 8 + store ptr null, ptr %__promise, align 8 + ret void +} + +declare void @use_value(ptr) From 2908155efeaa065c9c8f90e355ad4820195a7cc3 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Mon, 12 May 2025 11:13:24 +0800 Subject: [PATCH 02/10] Reformat --- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index ff1e1c8cc56cf..96ec2860f3853 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -9,7 +9,6 @@ #include "llvm/Transforms/Coroutines/CoroEarly.h" #include "CoroInternal.h" #include "llvm/IR/DIBuilder.h" -#include "llvm/IR/Dominators.h" #include "llvm/IR/Function.h" #include "llvm/IR/IRBuilder.h" #include "llvm/IR/InstIterator.h" @@ -264,9 +263,8 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { auto *Alignment = Builder.getInt32(PA->getAlign().value()); auto *FromPromise = Builder.getInt1(false); SmallVector Arg{CoroBegin, Alignment, FromPromise}; - auto *PI = - Builder.CreateIntrinsic(Builder.getPtrTy(), Intrinsic::coro_promise, - Arg, {}, "promise.addr"); + auto *PI = Builder.CreateIntrinsic( + Builder.getPtrTy(), Intrinsic::coro_promise, Arg, {}, "promise.addr"); PA->replaceUsesWithIf(PI, [CoroId](Use &U) { bool IsBitcast = U == U.getUser()->stripPointerCasts(); bool IsCoroId = U.getUser() == CoroId; From 28596aab28c5e53fca955aad61391ecb0abb89bb Mon Sep 17 00:00:00 2001 From: NewSigma Date: Mon, 12 May 2025 21:16:10 +0800 Subject: [PATCH 03/10] Handle cases that coro.promise may be inlined --- llvm/lib/Transforms/Coroutines/Coroutines.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index 325ad36bc666a..a7227224eb900 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -495,8 +495,10 @@ void coro::Shape::cleanCoroutine( CoroSave->eraseFromParent(); UnusedCoroSaves.clear(); + auto *AI = getPromiseAlloca(); for (auto *PI : CoroPromises) { - PI->replaceAllUsesWith(getPromiseAlloca()); + PI->replaceAllUsesWith(PI->isFromPromise() ? cast(CoroBegin) + : cast(AI)); PI->eraseFromParent(); } } From e1008dfe75dc46696186b372c99cebe4487fdd79 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Mon, 12 May 2025 21:52:50 +0800 Subject: [PATCH 04/10] Handle cases coro handle may be passed to another coroutine --- llvm/docs/Coroutines.rst | 6 +++--- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/llvm/docs/Coroutines.rst b/llvm/docs/Coroutines.rst index 157b20b7d8d73..f64029547e648 100644 --- a/llvm/docs/Coroutines.rst +++ b/llvm/docs/Coroutines.rst @@ -2123,9 +2123,9 @@ CoroEarly --------- The CoroEarly pass ensures later middle end passes correctly interpret coroutine semantics and lowers coroutine intrinsics that not needed to be preserved to -help later coroutine passes. This pass lowers `coro.promise`_ that outside the -coroutine body, `coro.frame`_ and `coro.done`_ intrinsics. It replace uses of -promise alloca with `coro.promise`_ intrinsic. +help later coroutine passes. This pass lowers `coro.promise`_, `coro.frame`_ and +`coro.done`_ intrinsics. Afterwards, it replace uses of promise alloca with +`coro.promise`_ intrinsic. .. _CoroSplit: diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index 96ec2860f3853..f5d21435cd6e8 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -237,9 +237,7 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { lowerResumeOrDestroy(*CB, CoroSubFnInst::DestroyIndex); break; case Intrinsic::coro_promise: { - bool OutsideCoro = CoroBegin == nullptr; - if (OutsideCoro) - lowerCoroPromise(cast(&I)); + lowerCoroPromise(cast(&I)); break; } case Intrinsic::coro_done: From cb7bb719f97f3b332cd645619b2e475aa60d3225 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Tue, 13 May 2025 17:35:53 +0800 Subject: [PATCH 05/10] Only emit coro.promise if CoroBegin is nonnull --- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 9 ++++----- llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll | 5 ++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index f5d21435cd6e8..dc1ca0e07434c 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -236,10 +236,9 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { case Intrinsic::coro_destroy: lowerResumeOrDestroy(*CB, CoroSubFnInst::DestroyIndex); break; - case Intrinsic::coro_promise: { + case Intrinsic::coro_promise: lowerCoroPromise(cast(&I)); break; - } case Intrinsic::coro_done: lowerCoroDone(cast(&I)); break; @@ -253,9 +252,9 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { for (CoroFreeInst *CF : CoroFrees) CF->setArgOperand(0, CoroId); - if (auto *PA = CoroId->getPromise()) { - assert(CoroBegin && "Use Switch-Resumed ABI but missing coro.begin"); - + auto *PA = CoroId->getPromise(); + if (PA && CoroBegin) { + assert(isa(PA) && "Must pass alloca to coro.id"); Builder.SetInsertPoint(*CoroBegin->getInsertionPointAfterDef()); auto *Alignment = Builder.getInt32(PA->getAlign().value()); diff --git a/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll index ed3af686d5257..65b27ee2241dd 100644 --- a/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll +++ b/llvm/test/Instrumentation/AddressSanitizer/skip-coro.ll @@ -15,9 +15,8 @@ define ptr @foo() #0 { entry: %__promise = alloca %struct.Promise, align 8 %0 = call token @llvm.coro.id(i32 16, ptr nonnull %__promise, ptr null, ptr null) - %1 = call ptr @llvm.coro.begin(token %0, ptr null) - %2 = call ptr @llvm.coro.noop() - ret ptr %2 + %1 = call ptr @llvm.coro.noop() + ret ptr %1 } declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr) From dc0bd0af7bf31eb6a4afe0b95a211a14b1f51b75 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Tue, 13 May 2025 21:41:08 +0800 Subject: [PATCH 06/10] Make use of uniqueness of coro.promise --- .../llvm/Transforms/Coroutines/CoroShape.h | 10 +++++----- llvm/lib/Transforms/Coroutines/Coroutines.cpp | 15 +++++++-------- llvm/test/Transforms/Coroutines/gh105595.ll | 1 - 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h index aee918946c70f..d4269176e8d57 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h @@ -80,7 +80,7 @@ struct Shape { // Scan the function and collect the above intrinsics for later processing void analyze(Function &F, SmallVectorImpl &CoroFrames, SmallVectorImpl &UnusedCoroSaves, - SmallVectorImpl &CoroPromises); + CoroPromiseInst *&CoroPromise); // If for some reason, we were not able to find coro.begin, bailout. void invalidateCoroutine(Function &F, SmallVectorImpl &CoroFrames); @@ -89,7 +89,7 @@ struct Shape { // Remove orphaned and unnecessary intrinsics void cleanCoroutine(SmallVectorImpl &CoroFrames, SmallVectorImpl &UnusedCoroSaves, - SmallVectorImpl &CoroPromises); + CoroPromiseInst *CoroPromise); // Field indexes for special fields in the switch lowering. struct SwitchFieldIndex { @@ -267,14 +267,14 @@ struct Shape { explicit Shape(Function &F) { SmallVector CoroFrames; SmallVector UnusedCoroSaves; - SmallVector CoroPromises; + CoroPromiseInst * CoroPromise = nullptr; - analyze(F, CoroFrames, UnusedCoroSaves, CoroPromises); + analyze(F, CoroFrames, UnusedCoroSaves, CoroPromise); if (!CoroBegin) { invalidateCoroutine(F, CoroFrames); return; } - cleanCoroutine(CoroFrames, UnusedCoroSaves, CoroPromises); + cleanCoroutine(CoroFrames, UnusedCoroSaves, CoroPromise); } }; diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index a7227224eb900..92b45a788a887 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -193,7 +193,7 @@ static CoroSaveInst *createCoroSave(CoroBeginInst *CoroBegin, void coro::Shape::analyze(Function &F, SmallVectorImpl &CoroFrames, SmallVectorImpl &UnusedCoroSaves, - SmallVectorImpl &CoroPromises) { + CoroPromiseInst *&CoroPromise) { clear(); bool HasFinalSuspend = false; @@ -288,7 +288,7 @@ void coro::Shape::analyze(Function &F, } break; case Intrinsic::coro_promise: - CoroPromises.push_back(cast(II)); + CoroPromise = cast(II); break; } } @@ -481,8 +481,7 @@ void coro::AnyRetconABI::init() { void coro::Shape::cleanCoroutine( SmallVectorImpl &CoroFrames, - SmallVectorImpl &UnusedCoroSaves, - SmallVectorImpl &CoroPromises) { + SmallVectorImpl &UnusedCoroSaves, CoroPromiseInst *PI) { // The coro.frame intrinsic is always lowered to the result of coro.begin. for (CoroFrameInst *CF : CoroFrames) { CF->replaceAllUsesWith(CoroBegin); @@ -495,10 +494,10 @@ void coro::Shape::cleanCoroutine( CoroSave->eraseFromParent(); UnusedCoroSaves.clear(); - auto *AI = getPromiseAlloca(); - for (auto *PI : CoroPromises) { - PI->replaceAllUsesWith(PI->isFromPromise() ? cast(CoroBegin) - : cast(AI)); + if (PI) { + PI->replaceAllUsesWith(PI->isFromPromise() + ? cast(CoroBegin) + : cast(getPromiseAlloca())); PI->eraseFromParent(); } } diff --git a/llvm/test/Transforms/Coroutines/gh105595.ll b/llvm/test/Transforms/Coroutines/gh105595.ll index cc23419bf7ff4..0efe21216e998 100644 --- a/llvm/test/Transforms/Coroutines/gh105595.ll +++ b/llvm/test/Transforms/Coroutines/gh105595.ll @@ -18,7 +18,6 @@ resume: br label %suspend suspend: - call i1 @llvm.coro.end(ptr null, i1 false, token none) ; load value when resume ; CHECK: %null = load ptr, ptr %promise.addr, align 8 %null = load ptr, ptr %__promise, align 8 From 4249143be905989f5effee9587f92f93289372d2 Mon Sep 17 00:00:00 2001 From: NewSigma Date: Tue, 13 May 2025 22:02:07 +0800 Subject: [PATCH 07/10] Format code --- llvm/include/llvm/Transforms/Coroutines/CoroShape.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h index d4269176e8d57..891774b446571 100644 --- a/llvm/include/llvm/Transforms/Coroutines/CoroShape.h +++ b/llvm/include/llvm/Transforms/Coroutines/CoroShape.h @@ -267,7 +267,7 @@ struct Shape { explicit Shape(Function &F) { SmallVector CoroFrames; SmallVector UnusedCoroSaves; - CoroPromiseInst * CoroPromise = nullptr; + CoroPromiseInst *CoroPromise = nullptr; analyze(F, CoroFrames, UnusedCoroSaves, CoroPromise); if (!CoroBegin) { From b7c158e3c37b43cb2fb844d19856d8ff39d3e2eb Mon Sep 17 00:00:00 2001 From: NewSigma Date: Wed, 14 May 2025 21:51:36 +0800 Subject: [PATCH 08/10] Refactor and add comment --- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 39 ++++++++++++-------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index dc1ca0e07434c..580e41a7cad0f 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -30,6 +30,7 @@ class Lowerer : public coro::LowererBase { void lowerCoroPromise(CoroPromiseInst *Intrin); void lowerCoroDone(IntrinsicInst *II); void lowerCoroNoop(IntrinsicInst *II); + void hidePromiseAlloca(CoroIdInst *CoroId, CoroBeginInst *CoroBegin); public: Lowerer(Module &M) @@ -153,6 +154,27 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) { II->eraseFromParent(); } +// Later middle-end passes will assume promise alloca dead after coroutine +// suspend, leading to misoptimizations. We hide promise alloca using +// coro.promise and will lower it back to alloca at CoroSplit. +void Lowerer::hidePromiseAlloca(CoroIdInst *CoroId, CoroBeginInst *CoroBegin) { + auto *PA = CoroId->getPromise(); + if (!PA || !CoroBegin) + return; + Builder.SetInsertPoint(*CoroBegin->getInsertionPointAfterDef()); + + auto *Alignment = Builder.getInt32(PA->getAlign().value()); + auto *FromPromise = Builder.getInt1(false); + SmallVector Arg{CoroBegin, Alignment, FromPromise}; + auto *PI = Builder.CreateIntrinsic( + Builder.getPtrTy(), Intrinsic::coro_promise, Arg, {}, "promise.addr"); + PA->replaceUsesWithIf(PI, [CoroId](Use &U) { + bool IsBitcast = U == U.getUser()->stripPointerCasts(); + bool IsCoroId = U.getUser() == CoroId; + return !IsBitcast && !IsCoroId; + }); +} + // Prior to CoroSplit, calls to coro.begin needs to be marked as NoDuplicate, // as CoroSplit assumes there is exactly one coro.begin. After CoroSplit, // NoDuplicate attribute will be removed from coro.begin otherwise, it will @@ -252,22 +274,7 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { for (CoroFreeInst *CF : CoroFrees) CF->setArgOperand(0, CoroId); - auto *PA = CoroId->getPromise(); - if (PA && CoroBegin) { - assert(isa(PA) && "Must pass alloca to coro.id"); - Builder.SetInsertPoint(*CoroBegin->getInsertionPointAfterDef()); - - auto *Alignment = Builder.getInt32(PA->getAlign().value()); - auto *FromPromise = Builder.getInt1(false); - SmallVector Arg{CoroBegin, Alignment, FromPromise}; - auto *PI = Builder.CreateIntrinsic( - Builder.getPtrTy(), Intrinsic::coro_promise, Arg, {}, "promise.addr"); - PA->replaceUsesWithIf(PI, [CoroId](Use &U) { - bool IsBitcast = U == U.getUser()->stripPointerCasts(); - bool IsCoroId = U.getUser() == CoroId; - return !IsBitcast && !IsCoroId; - }); - } + hidePromiseAlloca(CoroId, CoroBegin); } // Coroutine suspention could potentially lead to any argument modified From 8476d7ead88ef0bf9a42d380206c8020a08fb97c Mon Sep 17 00:00:00 2001 From: NewSigma Date: Thu, 15 May 2025 18:18:31 +0800 Subject: [PATCH 09/10] Add NoDuplicate to coro.promise --- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 1 + llvm/lib/Transforms/Coroutines/Coroutines.cpp | 2 ++ 2 files changed, 3 insertions(+) diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index 580e41a7cad0f..40544a9abb8c2 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -168,6 +168,7 @@ void Lowerer::hidePromiseAlloca(CoroIdInst *CoroId, CoroBeginInst *CoroBegin) { SmallVector Arg{CoroBegin, Alignment, FromPromise}; auto *PI = Builder.CreateIntrinsic( Builder.getPtrTy(), Intrinsic::coro_promise, Arg, {}, "promise.addr"); + PI->setCannotDuplicate(); PA->replaceUsesWithIf(PI, [CoroId](Use &U) { bool IsBitcast = U == U.getUser()->stripPointerCasts(); bool IsCoroId = U.getUser() == CoroId; diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp index 92b45a788a887..02500ff778b80 100644 --- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp +++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp @@ -288,6 +288,8 @@ void coro::Shape::analyze(Function &F, } break; case Intrinsic::coro_promise: + assert(CoroPromise == nullptr && + "CoroEarly must ensure coro.promise unique"); CoroPromise = cast(II); break; } From 71d0e865d4b5884744bec97e8520a25b3025357c Mon Sep 17 00:00:00 2001 From: NewSigma Date: Fri, 16 May 2025 12:09:34 +0800 Subject: [PATCH 10/10] Remove unnecessary code --- llvm/lib/Transforms/Coroutines/CoroEarly.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp index 40544a9abb8c2..eea6dfba14e37 100644 --- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp @@ -200,22 +200,12 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) { default: continue; case Intrinsic::coro_begin: - case Intrinsic::coro_begin_custom_abi: { - auto CBI = cast(&I); - - // Ignore coro id's that aren't pre-split. - auto Id = dyn_cast(CBI->getId()); - if (Id && !Id->getInfo().isPreSplit()) - break; - + case Intrinsic::coro_begin_custom_abi: if (CoroBegin) report_fatal_error( "coroutine should have exactly one defining @llvm.coro.begin"); - CBI->addRetAttr(Attribute::NonNull); - CBI->addRetAttr(Attribute::NoAlias); - CoroBegin = CBI; + CoroBegin = cast(&I); break; - } case Intrinsic::coro_free: CoroFrees.push_back(cast(&I)); break;