[PATCH] D23461: [Coroutines] Part 7: Split coroutine into subfunctions

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:22
@@ +21,3 @@
+#include "llvm/IR/IRBuilder.h"
+#include <limits>
+
----------------
Do we still need this include?

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:104
@@ +103,3 @@
+  PointerType *FramePtrTy = Shape.FrameTy->getPointerTo();
+  Instruction *FramePtr =
+      cast<Instruction>(Builder.CreateBitCast(CB, FramePtrTy, "FramePtr"));
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:156
@@ +155,3 @@
+  ValueToValueMapTy VMap;
+  // replace all args with undefs
+  for (Argument &A : F.getArgumentList())
----------------
A comment should be a sentence: start with a cap letter and end with a period.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:204-206
@@ +203,5 @@
+      case Intrinsic::coro_frame:
+        assert(CoroBegin && "coro.frame should not appear before coro.begin");
+        II->replaceAllUsesWith(CoroBegin);
+        II->eraseFromParent();
+        break;
----------------
This is not true, the basic blocks can be in a jumbled order.  If you want to do this sort of check, you need to use a depth first iterator like `depth_first` and turn this `assert` into a `report_fatal_error`.

Alternatively, you can save the coro.frame you come across for later processing instead of a dfs walk.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:209
@@ +208,3 @@
+      case Intrinsic::coro_suspend:
+        assert(CoroBegin && "coro.suspend should not appear before coro.begin");
+        CoroSuspends.push_back(cast<CoroSuspendInst>(II));
----------------
Ditto.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:226-227
@@ +225,4 @@
+        if (CB->getId()->getInfo().isPreSplit()) {
+          assert(!CoroBegin &&
+            "coroutine should have exactly one defining @llvm.coro.begin");
+          CB->addAttribute(AttributeSet::ReturnIndex, Attribute::NonNull);
----------------
I think this should be a report_fatal_error because nothing prohibits it upfront.


https://reviews.llvm.org/D23461