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

[Gor Nishanov via llvm-commits]

GorNishanov 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.
 
----------------
majnemer wrote:
> Hmm, coro.begin also takes the id.  `coro.free` claims it reads the frame parameter, would anything break if we stopped doing that?
I was thinking that if an allocation code gets unswitched we may get more than one coro.begin for the same coro.id and we would need to find all coro.begins and tied them back with a PHINode and use it as a replacement value for coro.free if heap elision is not performed.

Having it as a parameter allows LLVM to do it automatically for us.

================
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. 
+
----------------
majnemer wrote:
> This is a little strange...
It was one of the ways I thought of how we can detect whether a coro.id we are looking at describes the enclosing function or it describes a different function due to inline coroutine start function into its caller.

This used to be important in the design where I was stroring "elide-or-not" boolean in the coroutine frame. Since now we switched to separate f$destroy and f$cleanup, identifying whether coro.id corresponds to the current function or not is no longer important. 

I can take that parameter out altogether.

================
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;
----------------
majnemer wrote:
> This looks like a duplicated version of `Instruction::mayThrow`
Cool. I can inline this function into shouldElide as:

isa<ReturnInst> || mayThrow(T)

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:172-173
@@ +171,4 @@
+  for (BasicBlock &BB : *F) {
+    auto *T = BB.getTerminator();
+    if (!returnsOrUnwindsToCaller(T))
+      continue;
----------------
majnemer wrote:
> Terminators are not the only instruction which may throw.  Calls may throw as well.
After we talked on IRC, I am going to redo the shouldElide into something simpler. Something like:

If every coro.destroy points at coro.begin (that takes care of the escape, since if there is no direct SSA to coro.begin), it did escape and we were not able to put coro.begin in the vreg
<gor> majnemer: More precisely, if for every coro.begin I found a coro.destroy directly pointing at it, we are good for elision


https://reviews.llvm.org/D23844