[Coroutines][Swift] Remove replaceSwiftErrorOps while cloning

[TylerNowicki]

I am confused by what replaceSwiftErrorOps is supposed to do and it doesn't seem to be well covered by lit-tests. At least in tree.

The function appears to primarily operate on the original function, because it processes the SwiftErrorOps in Shape, collected from the unsplit function. However, it is called during cloning process of each resume function and from reading the code it seems to do something strange.

After cloning the first resume function it may add Load and Store instructions to the original function. These would then appear in any subsequent resume functions that are cloned from the original, but not the one being processed. Instead, an alloca will be created in the first resume function. After the first call to replaceSwiftErrorOps the SwiftErrorOps list is cleared so no other resume functions will get the alloca. Following this replaceSwiftErrorOps is called again after splitting but that would do nothing (right?). The original commit doesn't seem to shed any light on it [1].

Removing the call within the Cloner::create() doesn't break any lit-tests. Can this be safely removed?

I am looking at this because I am working on splitting. As explained in #116285 I want to CloneAndPrune to create resume functions that only include the code they need and not the entire original function. However, this call causes coro-swifterror.ll to fail by:

swifterror argument should come from an alloca or parameter ptr poison
tail call void @maybeThrow(ptr swifterror poison)

The swifterror argument is not correctly used in a few places in the IR and removing the call to replaceSwiftErrorOps() in Cloner::create() resolves the problem (the lit-test passes).

[1] 2133fee

[llvmbot]

@llvm/pr-subscribers-coroutines

Author: Tyler Nowicki (TylerNowicki)

Changes

I am confused by what replaceSwiftErrorOps is supposed to do and it doesn't seem to be well covered by lit-tests. At least in tree.

The function appears to primarily operate on the original function, because it processes the SwiftErrorOps in Shape, collected from the unsplit function. However, it is called during cloning process of each resume function and from reading the code it seems to do something strange.

After cloning the first resume function it may add Load and Store instructions to the original function. These would then appear in any subsequent resume functions that are cloned from the original, but not the one being processed. Instead, an alloca will be created in the first resume function. After the first call to replaceSwiftErrorOps the SwiftErrorOps list is cleared so no other resume functions will get the alloca. Following this replaceSwiftErrorOps is called again after splitting but that would do nothing (right?). The original commit doesn't seem to shed any light on it [1].

Removing the call within the Cloner::create() doesn't break any lit-tests. Can this be safely removed?

I am looking at this because I am working on splitting. As explained in #116285 I want to CloneAndPrune to create resume functions that only include the code they need and not the entire original function. However, this call causes coro-swifterror.ll to fail by:

swifterror argument should come from an alloca or parameter ptr poison
tail call void @maybeThrow(ptr swifterror poison)

The swifterror argument is not correctly used in a few places in the IR and removing the call to replaceSwiftErrorOps() in Cloner::create() resolves the problem (the lit-test passes).

[1] 2133fee

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (-8)

diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 25a962ddf1b0da..da2c66b1827cc4 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -190,7 +190,6 @@ class CoroCloner {
   void replaceRetconOrAsyncSuspendUses();
   void replaceCoroSuspends();
   void replaceCoroEnds();
-  void replaceSwiftErrorOps();
   void salvageDebugInfo();
   void handleFinalSuspend();
 };
@@ -750,10 +749,6 @@ collectDbgVariableIntrinsics(Function &F) {
   return {Intrinsics, DbgVariableRecords};
 }
 
-void CoroCloner::replaceSwiftErrorOps() {
-  ::replaceSwiftErrorOps(*NewF, Shape, &VMap);
-}
-
 void CoroCloner::salvageDebugInfo() {
   auto [Worklist, DbgVariableRecords] = collectDbgVariableIntrinsics(*NewF);
   SmallDenseMap<Argument *, AllocaInst *, 4> ArgToAllocaMap;
@@ -1204,9 +1199,6 @@ void CoroCloner::create() {
   // Handle suspends.
   replaceCoroSuspends();
 
-  // Handle swifterror.
-  replaceSwiftErrorOps();
-
   // Remove coro.end intrinsics.
   replaceCoroEnds();
 

[ChuanqiXu9]

I believe we need signoff from @rjmccall to continue in this direction.