[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-llvm-transforms

@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.

[rjmccall]

I can't tell you exactly how this code works, but I can tell you what it's trying to do. There are special rules for swifterror allocas, arguments, and parameters: a swifterror alloca or parameter can only be loaded, stored, or passed as a swifterror call argument, and a swifterror call argument can only be a direct reference to a swifterror alloca or parameter. These rules, not coincidentally, mean that you can always perfectly model the data flow in the alloca, and LLVM CodeGen actually has to do that in order to emit code. This is frankly a pretty bad representation, and we'd be better off making the data flow explicit, but it's what we've got right now.

Anyway, the problem for coro lowering is that the default treatment of allocas breaks those rules — splitting will try to replace the alloca with an entry in the coro frame, which can lead to trying to pass that as a swifterror argument. To pass a swifterror argument in a split function, we need to still have the alloca around; but we also potentially need the coro frame slot, since useful data can (in theory) be stored in the swifterror alloca slot across suspensions in the presplit coroutine. So I believe what this code is trying to do is have both of them and keep them in sync.

That said, I don't know why we'd need to do anything after splitting.

The simplest approach would be, all in the presplit function:

• don't remove the allocas
• don't replace any uses of the alloca with the coro frame slot
• make all stores to the alloca also update the coro frame slot
• copy the current value of the coro frame slot into its corresponding alloca after every suspension
  That should be good enough to keep them in sync.

A much better approach would be, again all in the presplit function:

• create a new swifterror alloca
• make the data flow explicit before doing the suspension analysis
  • loads and stores of the existing allocas will be removed
  • argument uses of the existing allocas will be replaced with the new one, which we store and load immediately around the call
• remove all the existing allocas, which should have no remaining uses
• the new alloca should only be used in ways that don't need to be live across suspensions, so simply ignore it in the suspension analysis

Neither of these approach requires emitting more instructions after split.

[TylerNowicki]

Thanks for the details! Do you know if it is possible to detect when swift is being used? I am refactoring the cloner to try to isolate the code required for each abi. #116885 I feel it would be best to isolate swift code in a similar fashion by inheriting the base cloner and implementing the changes needed for swift on top.

Also, perhaps you noticed the new custom ABI object I introduced earlier [1]? I believe that could be used to pull the swift code out of the LLVM repo entirely (mostly). Instead, the swift compiler would implement the changes it needs right in its own repo. At least it is worth a look to see if that is possible. I am not working on swift but perhaps you know someone who might be interested in looking at that?

[1] - https://llvm.org/docs/Coroutines.html#custom-abis-and-plugin-libraries

[rjmccall]

I don't think you can reliably detect when Swift is being used. Clang supports the swiftcall calling convention in many common targets, and somebody could call a function that takes a swifterror argument from a C++ coroutine. For better or worse, swifterror is a generic LLVM IR feature orthogonal to the choice of coroutine ABI, so even if we abstracted out the coroutine ABI lowering details, we'd still need swifterror support here.

[TylerNowicki]

Okay, I understand. Considering that swifterror is a generic LLVM IR feature I think it should be discussed in the documentation [1]. I will create a documentation PR with some of the above info on swifterror.

I think swifterror is related to the intrinsics coro.alloca.alloc, coro.alloca.get and coro.alloca.free but these are also not described in [1]. Can you comment on their connection?

[1] - https://llvm.org/docs/Coroutines.html

[TylerNowicki]

See the documentation PR #117183

[TylerNowicki]

I will close this in favor of #117183