[llvm] [Coroutines] Refactor CoroShape::buildFrom for future use by ABI objects (PR #108623)
Tyler Nowicki via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 16 15:07:32 PDT 2024
https://github.com/TylerNowicki updated https://github.com/llvm/llvm-project/pull/108623
>From ba9e523436b62f4f683c14a5f5b7b5b089bd5ac9 Mon Sep 17 00:00:00 2001
From: tnowicki <tnowicki.nowicki at amd.com>
Date: Sat, 24 Aug 2024 02:33:38 -0400
Subject: [PATCH 1/2] [Coroutines] Refactor CoroShape::buildFrom to support ABI
* Refactor buildFrom to separate the analysis, abi related operations,
tidying and bailout.
* In a follow-up PR the code in initABI will be moved to an ABI object
init method. And the Shape constructor will no longer perform any
lowering, instead it will just call analysis. This will make the Shape
object a bit more useful because it can be constructed and used
anywhere. It may even be useful to make it an analysis pass.
* In a follow-up PR the OptimizeFrame flag will also be removed from the
Shape and instead will be passed directly to buildCoroutineFrame
(although it would be nice to find another way to trigger this
optimization). This is the only thing that Shape cannot determine from
the Function/Coroutine, but it is only needed within
buildCoroutineFrame.
* Note, that it was necessary to introduce two new SmallVectors, one to
track CoroFrames and the other for UnusedCoroSaves. The tidyCoroutine
method requires both, while invalidateCoroutine (bailout) method just
requires the former.
See RFC for more info: https://discourse.llvm.org/t/rfc-abi-objects-for-coroutines/81057
---
llvm/lib/Transforms/Coroutines/CoroShape.h | 55 ++++++-
llvm/lib/Transforms/Coroutines/Coroutines.cpp | 137 +++++++++---------
2 files changed, 117 insertions(+), 75 deletions(-)
diff --git a/llvm/lib/Transforms/Coroutines/CoroShape.h b/llvm/lib/Transforms/Coroutines/CoroShape.h
index f5798b63bf7325..3d1b38082173d1 100644
--- a/llvm/lib/Transforms/Coroutines/CoroShape.h
+++ b/llvm/lib/Transforms/Coroutines/CoroShape.h
@@ -50,15 +50,49 @@ enum class ABI {
// Holds structural Coroutine Intrinsics for a particular function and other
// values used during CoroSplit pass.
struct LLVM_LIBRARY_VISIBILITY Shape {
- CoroBeginInst *CoroBegin;
+ CoroBeginInst *CoroBegin = nullptr;
SmallVector<AnyCoroEndInst *, 4> CoroEnds;
SmallVector<CoroSizeInst *, 2> CoroSizes;
SmallVector<CoroAlignInst *, 2> CoroAligns;
SmallVector<AnyCoroSuspendInst *, 4> CoroSuspends;
- SmallVector<CallInst *, 2> SwiftErrorOps;
SmallVector<CoroAwaitSuspendInst *, 4> CoroAwaitSuspends;
SmallVector<CallInst *, 2> SymmetricTransfers;
+ // Values invalidated by invalidateCoroutine() and tidyCoroutine()
+ SmallVector<CoroFrameInst *, 8> CoroFrames;
+ SmallVector<CoroSaveInst *, 2> UnusedCoroSaves;
+
+ // Values invalidated by replaceSwiftErrorOps()
+ SmallVector<CallInst *, 2> SwiftErrorOps;
+
+ void clear() {
+ CoroBegin = nullptr;
+ CoroEnds.clear();
+ CoroSizes.clear();
+ CoroAligns.clear();
+ CoroSuspends.clear();
+ CoroAwaitSuspends.clear();
+ SymmetricTransfers.clear();
+
+ CoroFrames.clear();
+ UnusedCoroSaves.clear();
+
+ SwiftErrorOps.clear();
+
+ FrameTy = nullptr;
+ FramePtr = nullptr;
+ AllocaSpillBlock = nullptr;
+ }
+
+ // Scan the function and collect the above intrinsics for later processing
+ void analyze(Function &F);
+ // If for some reason, we were not able to find coro.begin, bailout.
+ void invalidateCoroutine(Function &F);
+ // Perform ABI related initial transformation
+ void initABI();
+ // Remove orphaned and unnecessary intrinsics
+ void tidyCoroutine();
+
// Field indexes for special fields in the switch lowering.
struct SwitchFieldIndex {
enum {
@@ -76,11 +110,11 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
coro::ABI ABI;
- StructType *FrameTy;
+ StructType *FrameTy = nullptr;
Align FrameAlign;
- uint64_t FrameSize;
- Value *FramePtr;
- BasicBlock *AllocaSpillBlock;
+ uint64_t FrameSize = 0;
+ Value *FramePtr = nullptr;
+ BasicBlock *AllocaSpillBlock = nullptr;
/// This would only be true if optimization are enabled.
bool OptimizeFrame;
@@ -237,9 +271,14 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
Shape() = default;
explicit Shape(Function &F, bool OptimizeFrame = false)
: OptimizeFrame(OptimizeFrame) {
- buildFrom(F);
+ analyze(F);
+ if (!CoroBegin) {
+ invalidateCoroutine(F);
+ return;
+ }
+ initABI();
+ tidyCoroutine();
}
- void buildFrom(Function &F);
};
} // end namespace coro
diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp
index 5cc13a584aef32..c1042b21883f67 100644
--- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp
+++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp
@@ -177,17 +177,6 @@ void coro::suppressCoroAllocs(LLVMContext &Context,
}
}
-static void clear(coro::Shape &Shape) {
- Shape.CoroBegin = nullptr;
- Shape.CoroEnds.clear();
- Shape.CoroSizes.clear();
- Shape.CoroSuspends.clear();
-
- Shape.FrameTy = nullptr;
- Shape.FramePtr = nullptr;
- Shape.AllocaSpillBlock = nullptr;
-}
-
static CoroSaveInst *createCoroSave(CoroBeginInst *CoroBegin,
CoroSuspendInst *SuspendInst) {
Module *M = SuspendInst->getModule();
@@ -200,13 +189,12 @@ static CoroSaveInst *createCoroSave(CoroBeginInst *CoroBegin,
}
// Collect "interesting" coroutine intrinsics.
-void coro::Shape::buildFrom(Function &F) {
+void coro::Shape::analyze(Function &F) {
+ clear();
+
bool HasFinalSuspend = false;
bool HasUnwindCoroEnd = false;
size_t FinalSuspendIndex = 0;
- clear(*this);
- SmallVector<CoroFrameInst *, 8> CoroFrames;
- SmallVector<CoroSaveInst *, 2> UnusedCoroSaves;
for (Instruction &I : instructions(F)) {
// FIXME: coro_await_suspend_* are not proper `IntrinisicInst`s
@@ -298,8 +286,58 @@ void coro::Shape::buildFrom(Function &F) {
}
}
- // If for some reason, we were not able to find coro.begin, bailout.
- if (!CoroBegin) {
+ // If there is no CoroBegin then this is not a coroutine.
+ if (!CoroBegin)
+ return;
+
+ // Determination of ABI and initializing lowering info
+ auto Id = CoroBegin->getId();
+ auto IntrID = Id->getIntrinsicID();
+ if (IntrID == Intrinsic::coro_id) {
+ ABI = coro::ABI::Switch;
+ SwitchLowering.HasFinalSuspend = HasFinalSuspend;
+ SwitchLowering.HasUnwindCoroEnd = HasUnwindCoroEnd;
+
+ auto SwitchId = getSwitchCoroId();
+ SwitchLowering.ResumeSwitch = nullptr;
+ SwitchLowering.PromiseAlloca = SwitchId->getPromise();
+ SwitchLowering.ResumeEntryBlock = nullptr;
+
+ // Move final suspend to the last element in the CoroSuspends vector.
+ if (SwitchLowering.HasFinalSuspend &&
+ FinalSuspendIndex != CoroSuspends.size() - 1)
+ std::swap(CoroSuspends[FinalSuspendIndex], CoroSuspends.back());
+ } else if (IntrID == Intrinsic::coro_id_async) {
+ ABI = coro::ABI::Async;
+ auto *AsyncId = getAsyncCoroId();
+ AsyncId->checkWellFormed();
+ AsyncLowering.Context = AsyncId->getStorage();
+ AsyncLowering.ContextArgNo = AsyncId->getStorageArgumentIndex();
+ AsyncLowering.ContextHeaderSize = AsyncId->getStorageSize();
+ AsyncLowering.ContextAlignment = AsyncId->getStorageAlignment().value();
+ AsyncLowering.AsyncFuncPointer = AsyncId->getAsyncFunctionPointer();
+ AsyncLowering.AsyncCC = F.getCallingConv();
+ } else if (IntrID == Intrinsic::coro_id_retcon ||
+ IntrID == Intrinsic::coro_id_retcon_once) {
+ ABI = IntrID == Intrinsic::coro_id_retcon ? coro::ABI::Retcon
+ : coro::ABI::RetconOnce;
+ auto ContinuationId = getRetconCoroId();
+ ContinuationId->checkWellFormed();
+ auto Prototype = ContinuationId->getPrototype();
+ RetconLowering.ResumePrototype = Prototype;
+ RetconLowering.Alloc = ContinuationId->getAllocFunction();
+ RetconLowering.Dealloc = ContinuationId->getDeallocFunction();
+ RetconLowering.ReturnBlock = nullptr;
+ RetconLowering.IsFrameInlineInStorage = false;
+ } else {
+ llvm_unreachable("coro.begin is not dependent on a coro.id call");
+ }
+}
+
+// If for some reason, we were not able to find coro.begin, bailout.
+void coro::Shape::invalidateCoroutine(Function &F) {
+ assert(!CoroBegin);
+ {
// Replace coro.frame which are supposed to be lowered to the result of
// coro.begin with undef.
auto *Undef = UndefValue::get(PointerType::get(F.getContext(), 0));
@@ -320,21 +358,13 @@ void coro::Shape::buildFrom(Function &F) {
// Replace all coro.ends with unreachable instruction.
for (AnyCoroEndInst *CE : CoroEnds)
changeToUnreachable(CE);
-
- return;
}
+}
- auto Id = CoroBegin->getId();
- switch (auto IdIntrinsic = Id->getIntrinsicID()) {
- case Intrinsic::coro_id: {
- auto SwitchId = cast<CoroIdInst>(Id);
- this->ABI = coro::ABI::Switch;
- this->SwitchLowering.HasFinalSuspend = HasFinalSuspend;
- this->SwitchLowering.HasUnwindCoroEnd = HasUnwindCoroEnd;
- this->SwitchLowering.ResumeSwitch = nullptr;
- this->SwitchLowering.PromiseAlloca = SwitchId->getPromise();
- this->SwitchLowering.ResumeEntryBlock = nullptr;
-
+// Perform semantic checking and initialization of the ABI
+void coro::Shape::initABI() {
+ switch (ABI) {
+ case coro::ABI::Switch: {
for (auto *AnySuspend : CoroSuspends) {
auto Suspend = dyn_cast<CoroSuspendInst>(AnySuspend);
if (!Suspend) {
@@ -349,33 +379,11 @@ void coro::Shape::buildFrom(Function &F) {
}
break;
}
- case Intrinsic::coro_id_async: {
- auto *AsyncId = cast<CoroIdAsyncInst>(Id);
- AsyncId->checkWellFormed();
- this->ABI = coro::ABI::Async;
- this->AsyncLowering.Context = AsyncId->getStorage();
- this->AsyncLowering.ContextArgNo = AsyncId->getStorageArgumentIndex();
- this->AsyncLowering.ContextHeaderSize = AsyncId->getStorageSize();
- this->AsyncLowering.ContextAlignment =
- AsyncId->getStorageAlignment().value();
- this->AsyncLowering.AsyncFuncPointer = AsyncId->getAsyncFunctionPointer();
- this->AsyncLowering.AsyncCC = F.getCallingConv();
+ case coro::ABI::Async: {
break;
};
- case Intrinsic::coro_id_retcon:
- case Intrinsic::coro_id_retcon_once: {
- auto ContinuationId = cast<AnyCoroIdRetconInst>(Id);
- ContinuationId->checkWellFormed();
- this->ABI = (IdIntrinsic == Intrinsic::coro_id_retcon
- ? coro::ABI::Retcon
- : coro::ABI::RetconOnce);
- auto Prototype = ContinuationId->getPrototype();
- this->RetconLowering.ResumePrototype = Prototype;
- this->RetconLowering.Alloc = ContinuationId->getAllocFunction();
- this->RetconLowering.Dealloc = ContinuationId->getDeallocFunction();
- this->RetconLowering.ReturnBlock = nullptr;
- this->RetconLowering.IsFrameInlineInStorage = false;
-
+ case coro::ABI::Retcon:
+ case coro::ABI::RetconOnce: {
// Determine the result value types, and make sure they match up with
// the values passed to the suspends.
auto ResultTys = getRetconResultTypes();
@@ -408,7 +416,7 @@ void coro::Shape::buildFrom(Function &F) {
#ifndef NDEBUG
Suspend->dump();
- Prototype->getFunctionType()->dump();
+ RetconLowering.ResumePrototype->getFunctionType()->dump();
#endif
report_fatal_error("argument to coro.suspend.retcon does not "
"match corresponding prototype function result");
@@ -417,14 +425,14 @@ void coro::Shape::buildFrom(Function &F) {
if (SI != SE || RI != RE) {
#ifndef NDEBUG
Suspend->dump();
- Prototype->getFunctionType()->dump();
+ RetconLowering.ResumePrototype->getFunctionType()->dump();
#endif
report_fatal_error("wrong number of arguments to coro.suspend.retcon");
}
// Check that the result type of the suspend matches the resume types.
Type *SResultTy = Suspend->getType();
- ArrayRef<Type*> SuspendResultTys;
+ ArrayRef<Type *> SuspendResultTys;
if (SResultTy->isVoidTy()) {
// leave as empty array
} else if (auto SResultStructTy = dyn_cast<StructType>(SResultTy)) {
@@ -436,7 +444,7 @@ void coro::Shape::buildFrom(Function &F) {
if (SuspendResultTys.size() != ResumeTys.size()) {
#ifndef NDEBUG
Suspend->dump();
- Prototype->getFunctionType()->dump();
+ RetconLowering.ResumePrototype->getFunctionType()->dump();
#endif
report_fatal_error("wrong number of results from coro.suspend.retcon");
}
@@ -444,7 +452,7 @@ void coro::Shape::buildFrom(Function &F) {
if (SuspendResultTys[I] != ResumeTys[I]) {
#ifndef NDEBUG
Suspend->dump();
- Prototype->getFunctionType()->dump();
+ RetconLowering.ResumePrototype->getFunctionType()->dump();
#endif
report_fatal_error("result from coro.suspend.retcon does not "
"match corresponding prototype function param");
@@ -453,23 +461,18 @@ void coro::Shape::buildFrom(Function &F) {
}
break;
}
-
default:
llvm_unreachable("coro.begin is not dependent on a coro.id call");
}
+}
+void coro::Shape::tidyCoroutine() {
// The coro.free intrinsic is always lowered to the result of coro.begin.
for (CoroFrameInst *CF : CoroFrames) {
CF->replaceAllUsesWith(CoroBegin);
CF->eraseFromParent();
}
- // Move final suspend to be the last element in the CoroSuspends vector.
- if (ABI == coro::ABI::Switch &&
- SwitchLowering.HasFinalSuspend &&
- FinalSuspendIndex != CoroSuspends.size() - 1)
- std::swap(CoroSuspends[FinalSuspendIndex], CoroSuspends.back());
-
// Remove orphaned coro.saves.
for (CoroSaveInst *CoroSave : UnusedCoroSaves)
CoroSave->eraseFromParent();
>From a7a1f180213a828aa063d450fd873fb5b791cc40 Mon Sep 17 00:00:00 2001
From: tnowicki <tnowicki.nowicki at amd.com>
Date: Mon, 16 Sep 2024 18:07:19 -0400
Subject: [PATCH 2/2] [Coroutines] Revise with reviewer feedback.
---
llvm/lib/Transforms/Coroutines/CoroShape.h | 6 ++---
llvm/lib/Transforms/Coroutines/Coroutines.cpp | 26 +++++++++++++------
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/llvm/lib/Transforms/Coroutines/CoroShape.h b/llvm/lib/Transforms/Coroutines/CoroShape.h
index 3d1b38082173d1..073677bd9638b4 100644
--- a/llvm/lib/Transforms/Coroutines/CoroShape.h
+++ b/llvm/lib/Transforms/Coroutines/CoroShape.h
@@ -58,7 +58,7 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
SmallVector<CoroAwaitSuspendInst *, 4> CoroAwaitSuspends;
SmallVector<CallInst *, 2> SymmetricTransfers;
- // Values invalidated by invalidateCoroutine() and tidyCoroutine()
+ // Values invalidated by invalidateCoroutine() and cleanCoroutine()
SmallVector<CoroFrameInst *, 8> CoroFrames;
SmallVector<CoroSaveInst *, 2> UnusedCoroSaves;
@@ -91,7 +91,7 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
// Perform ABI related initial transformation
void initABI();
// Remove orphaned and unnecessary intrinsics
- void tidyCoroutine();
+ void cleanCoroutine();
// Field indexes for special fields in the switch lowering.
struct SwitchFieldIndex {
@@ -277,7 +277,7 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
return;
}
initABI();
- tidyCoroutine();
+ cleanCoroutine();
}
};
diff --git a/llvm/lib/Transforms/Coroutines/Coroutines.cpp b/llvm/lib/Transforms/Coroutines/Coroutines.cpp
index c1042b21883f67..cb0a1bc26482c5 100644
--- a/llvm/lib/Transforms/Coroutines/Coroutines.cpp
+++ b/llvm/lib/Transforms/Coroutines/Coroutines.cpp
@@ -292,8 +292,8 @@ void coro::Shape::analyze(Function &F) {
// Determination of ABI and initializing lowering info
auto Id = CoroBegin->getId();
- auto IntrID = Id->getIntrinsicID();
- if (IntrID == Intrinsic::coro_id) {
+ switch (auto IntrID = Id->getIntrinsicID()) {
+ case Intrinsic::coro_id: {
ABI = coro::ABI::Switch;
SwitchLowering.HasFinalSuspend = HasFinalSuspend;
SwitchLowering.HasUnwindCoroEnd = HasUnwindCoroEnd;
@@ -307,7 +307,9 @@ void coro::Shape::analyze(Function &F) {
if (SwitchLowering.HasFinalSuspend &&
FinalSuspendIndex != CoroSuspends.size() - 1)
std::swap(CoroSuspends[FinalSuspendIndex], CoroSuspends.back());
- } else if (IntrID == Intrinsic::coro_id_async) {
+ break;
+ }
+ case Intrinsic::coro_id_async: {
ABI = coro::ABI::Async;
auto *AsyncId = getAsyncCoroId();
AsyncId->checkWellFormed();
@@ -317,8 +319,10 @@ void coro::Shape::analyze(Function &F) {
AsyncLowering.ContextAlignment = AsyncId->getStorageAlignment().value();
AsyncLowering.AsyncFuncPointer = AsyncId->getAsyncFunctionPointer();
AsyncLowering.AsyncCC = F.getCallingConv();
- } else if (IntrID == Intrinsic::coro_id_retcon ||
- IntrID == Intrinsic::coro_id_retcon_once) {
+ break;
+ }
+ case Intrinsic::coro_id_retcon:
+ case Intrinsic::coro_id_retcon_once: {
ABI = IntrID == Intrinsic::coro_id_retcon ? coro::ABI::Retcon
: coro::ABI::RetconOnce;
auto ContinuationId = getRetconCoroId();
@@ -329,7 +333,9 @@ void coro::Shape::analyze(Function &F) {
RetconLowering.Dealloc = ContinuationId->getDeallocFunction();
RetconLowering.ReturnBlock = nullptr;
RetconLowering.IsFrameInlineInStorage = false;
- } else {
+ break;
+ }
+ default:
llvm_unreachable("coro.begin is not dependent on a coro.id call");
}
}
@@ -345,6 +351,7 @@ void coro::Shape::invalidateCoroutine(Function &F) {
CF->replaceAllUsesWith(Undef);
CF->eraseFromParent();
}
+ CoroFrames.clear();
// Replace all coro.suspend with undef and remove related coro.saves if
// present.
@@ -354,6 +361,7 @@ void coro::Shape::invalidateCoroutine(Function &F) {
if (auto *CoroSave = CS->getCoroSave())
CoroSave->eraseFromParent();
}
+ CoroSuspends.clear();
// Replace all coro.ends with unreachable instruction.
for (AnyCoroEndInst *CE : CoroEnds)
@@ -466,16 +474,18 @@ void coro::Shape::initABI() {
}
}
-void coro::Shape::tidyCoroutine() {
- // The coro.free intrinsic is always lowered to the result of coro.begin.
+void coro::Shape::cleanCoroutine() {
+ // The coro.frame intrinsic is always lowered to the result of coro.begin.
for (CoroFrameInst *CF : CoroFrames) {
CF->replaceAllUsesWith(CoroBegin);
CF->eraseFromParent();
}
+ CoroFrames.clear();
// Remove orphaned coro.saves.
for (CoroSaveInst *CoroSave : UnusedCoroSaves)
CoroSave->eraseFromParent();
+ UnusedCoroSaves.clear();
}
static void propagateCallAttrsFromCallee(CallInst *Call, Function *Callee) {
More information about the llvm-commits
mailing list