[PATCH] D75664: [Coroutines] Also check lifetime intrinsic for local variable when build coroutine frame

[Brian Gesiak via Phabricator via llvm-commits]

modocache accepted this revision.
modocache added a comment.

Nice, thanks for this improvement! Am I right in thinking patch is dependent upon D76119 <https://reviews.llvm.org/D76119>? My understanding is that, without that patch, then we wouldn't be able to rely on lifetime intrinsics when using `-O0`. You may want to use Phabricator's "Edit Related Objects" link to specify that this patch depends on that one.

This LGTM assuming we can rely on lifetime intrinsics being generated at `-O0`. I'll comment on D76119 <https://reviews.llvm.org/D76119> on that point.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1470
+            break;
+        }
+      else
----------------
Nit-pick: I think the LLVM coding style would have you remove these `for` loop braces. There's only a single statement in the `for` loop, the `if` statement above, so you technically don't need these here.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:572
   Entry->setName("entry" + Suffix);
-  Entry->moveBefore(&NewF->getEntryBlock());
+  Entry->moveBefore(OldEntry);
   Entry->getTerminator()->eraseFromParent();
----------------
Wow, I'm surprised `coro-split-02.ll` was the only test case you ended up needing to modify. I would have thought that this one change would have broken a lot more tests' assertions.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:584
 
-  // TODO: move any allocas into Entry that weren't moved into the frame.
-  // (Currently we move all allocas into the frame.)
----------------
Awesome! Nice to remove this TODO.


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

https://reviews.llvm.org/D75664