[llvm] [Coroutines][Swift] Remove replaceSwiftErrorOps while cloning (PR #116292)

John McCall via llvm-commits llvm-commits at lists.llvm.org
Sun Nov 17 22:59:55 PST 2024


rjmccall wrote:

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 `swift error` 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.

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


More information about the llvm-commits mailing list