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

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroCleanup.cpp:30-37
@@ +29,10 @@
+
+static void simplifyCFG(Function &F) {
+  llvm::legacy::FunctionPassManager FPM(F.getParent());
+  FPM.add(createCFGSimplificationPass());
+
+  FPM.doInitialization();
+  FPM.run(F);
+  FPM.doFinalization();
+}
+
----------------
I'd just make `simplifyFunctionCFG` a non-internal function and call it instead of making a new FPM.

================
Comment at: lib/Transforms/Coroutines/CoroCleanup.cpp:44
@@ +43,3 @@
+    Instruction &I = *IB++;
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I)) {
+      switch (II->getIntrinsicID()) {
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroCleanup.cpp:44
@@ +43,3 @@
+    Instruction &I = *IB++;
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I)) {
+      switch (II->getIntrinsicID()) {
----------------
majnemer wrote:
> `auto *`
Should this be a `CallSite` instead?

================
Comment at: lib/Transforms/Coroutines/CoroEarly.cpp:52
@@ +51,3 @@
+static void setCannotDuplicate(CoroIdInst *CoroId) {
+  for (User* U : CoroId->users())
+    if (auto *CB = dyn_cast<CoroBeginInst>(U))
----------------
Pointers lean right.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:29
@@ +28,3 @@
+static BasicBlock *splitBlockIfNotFirst(Instruction *I, const Twine &Name) {
+  auto BB = I->getParent();
+  if (&*BB->begin() == I) {
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:30
@@ +29,3 @@
+  auto BB = I->getParent();
+  if (&*BB->begin() == I) {
+    if (BB->getSinglePredecessor()) {
----------------
You could use `&BB->front()`.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:46
@@ +45,3 @@
+
+// TODO: Implement in future patches
+struct SpillInfo {};
----------------
Please have a period at the end of this comment.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:63-66
@@ +62,6 @@
+  StructType *FrameTy = StructType::create(C, Name);
+  auto FramePtrTy = FrameTy->getPointerTo();
+  auto FnTy = FunctionType::get(Type::getVoidTy(C), FramePtrTy,
+                                /*IsVarArgs=*/false);
+  auto FnPtrTy = FnTy->getPointerTo();
+
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:68
@@ +67,3 @@
+
+  if (Shape.CoroSuspends.size() > std::numeric_limits<uint32_t>::max())
+    report_fatal_error("Cannot handle coroutine with this many suspend points");
----------------
We aren't huge users of `std::numeric_limits`, `UINT32_MAX` is a little more terse.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:101
@@ +100,3 @@
+static Instruction *insertSpills(SpillInfo &Spills, coro::Shape &Shape) {
+  auto CB = Shape.CoroBegin;
+  IRBuilder<> Builder(CB->getNextNode());
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:109
@@ +108,3 @@
+
+  auto FramePtrBB = FramePtr->getParent();
+  Shape.AllocaSpillBlock =
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:196
@@ +195,3 @@
+  for (auto IB = inst_begin(F), IE = inst_end(F); IB != IE;) {
+    if (auto II = dyn_cast<IntrinsicInst>(&*IB++)) {
+      switch (II->getIntrinsicID()) {
----------------
`auto *` although maybe we should use a CallSite?


https://reviews.llvm.org/D23461