[PATCH] D23844: [Coroutines] Part 9: Add cleanup subfunction.

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: docs/Coroutines.rst:806-810
@@ -805,4 +805,7 @@
 
-A pointer to the coroutine frame. This should be the same pointer that was 
-returned by prior `coro.begin` call.
+The first argument is a token returned by a call to '``llvm.coro.id``' 
+identifying the coroutine.
+
+The second argument is a pointer to the coroutine frame. This should be the same
+pointer that was returned by prior `coro.begin` call.
 
----------------
Hmm, coro.begin also takes the id.  `coro.free` claims it reads the frame parameter, would anything break if we stopped doing that?

================
Comment at: docs/Coroutines.rst:934-935
@@ -929,2 +933,4 @@
 
-The third argument is `null` before coroutine is split, and later is replaced 
+The third argument is `null` coming out of the frontend. The CoroEarly pass sets
+this argument to point to the function this coro.id belongs to. 
+
----------------
This is a little strange...

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:150-158
@@ +149,11 @@
+// leaves the function in some way.
+static bool returnsOrUnwindsToCaller(TerminatorInst *T) {
+  if (isa<ReturnInst>(T) || isa<ResumeInst>(T))
+    return true;
+
+  if (auto *CR = dyn_cast<CleanupReturnInst>(T))
+    return CR->unwindsToCaller();
+  if (auto *CS = dyn_cast<CatchSwitchInst>(T))
+    return CS->unwindsToCaller();
+
+  return false;
----------------
This looks like a duplicated version of `Instruction::mayThrow`

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:172-173
@@ +171,4 @@
+  for (BasicBlock &BB : *F) {
+    auto *T = BB.getTerminator();
+    if (!returnsOrUnwindsToCaller(T))
+      continue;
----------------
Terminators are not the only instruction which may throw.  Calls may throw as well.


https://reviews.llvm.org/D23844