[PATCH] D108122: FunctionAttrs: do not mark coroutines with odd return path `noreturn`

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

Well, I think the basic problem is that unlowered coroutines cannot be modeled as normal functions very well in LLVM IR, and the coro intrinsics are doing their best to muddle through given the unstated requirement to not bother anyone else too much.  We have major representational problems with modeling some of the things we need, especially the distinctions between ordinary-ish behavior in the ramp function, logically separate from the coroutine, vs. things that are semantically part of the coroutine body.

`coro.end` is one place where the representation is especially awkward, in part because of contrasting requirements between different lowerings.

- In the the retcon lowerings, `coro.end` is always an immediate exit from the coroutine, akin to `ret`.  In a more abstract representation, this could just be a `ret void` (because the semantic return type of a `retcon` coroutine is always `void`, at least for now).  However, actually using `ret` would be awkward in the current representation because we'd need to give it special treatment *without* giving special treatment to `ret`s in the ramp portion of the function.

- For the async lowering, `coro.end` was totally inadequate for what we needed to express, and we had to introduce a different intrinsic.  This is because the way we "return" to our async caller is by performing a tail call to its continuation function, and we need to represent both the function pointer and the return values.  In a more abstract representation, these things could be done implicitly, like ordinary CC lowering, and this could just be a `ret`.  I think part of the reason we went this way is because we wanted to give ourselves room to change around how things like the continuation function pointer were stored (e.g. with pointer signing), and we weren't sure if we'd ever want to "return" by tail calling something that wasn't the normal continuation function pointer.  We also do need to represent an awful lot of details about the async CC, like the allocation of async frames, so it makes some sense to be consistently less abstract.

- In the switch lowering, `coro.end` is not necessarily an immediate end of the coroutine, at least when it appears in cleanups.  I don't really understand why the representation works the way it does here, to be honest; I think Gor might not have been certain how else to get Clang to emit the IR pattern he wanted, which is not a very good reason.

Unless you really want to put a lot of work into making a more abstract representation, I think not inferring any function attributes for unlowered coroutines is probably the most reasonable thing to do.  IIRC, the way it currently works is that the coroutine lowering passes just remove these attributes, but that's always been questionable, not least because it's possible they'll have caused misbehavior before then.  Note that you really shouldn't infer *any* function attributes, including things like `readonly`, because there's a lot of implicit work that can get added during lowering.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108122/new/

https://reviews.llvm.org/D108122