[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:26-44
@@ +25,21 @@
+
+// Splits the block at a particular instruction unless it is the first
+// instruction in the block with a single predecessor.
+static BasicBlock *splitBlockIfNotFirst(Instruction *I, const Twine &Name) {
+  auto *BB = I->getParent();
+  if (&BB->front() == I) {
+    if (BB->getSinglePredecessor()) {
+      BB->setName(Name);
+      return BB;
+    }
+  }
+  return BB->splitBasicBlock(I, Name);
+}
+
+// Split above and below a particular instruction so that it
+// will be all alone by itself in a block.
+static void splitAround(Instruction *I, const Twine &Name) {
+  splitBlockIfNotFirst(I, Name);
+  splitBlockIfNotFirst(I->getNextNode(), "After" + Name);
+}
+
----------------
These functions seem dead.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:132
@@ +131,3 @@
+
+  // remove the rest of the block, by splitting it into an unreachable block
+  auto *BB = NewE->getParent();
----------------
Caps + period.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:156
@@ +155,3 @@
+  ValueToValueMapTy VMap;
+  // Replace all args with undefs.
+  for (Argument &A : F.getArgumentList())
----------------
Hmm, do you have a comment somewhere explaining why? Is it expected that there are no meaningful users of the args?

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:172
@@ +171,3 @@
+
+  // Remove old returns
+  for (ReturnInst *Return : Returns) {
----------------
Period.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:174-175
@@ +173,4 @@
+  for (ReturnInst *Return : Returns) {
+    new UnreachableInst(C, Return);
+    Return->eraseFromParent();
+  }
----------------
You could use `changeToUnreachable`.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:185
@@ +184,3 @@
+
+  // Make AllocaSpillBlock the new entry block
+  auto *SwitchBB = cast<BasicBlock>(VMap[ResumeEntry]);
----------------
Ditto.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:271
@@ +270,3 @@
+  SmallVector<Constant *, 4> Args(Fns.begin(), Fns.end());
+  assert(Args.size() > 0);
+  Function *Part = *Fns.begin();
----------------
`Args.empty()`.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:281
@@ +280,3 @@
+
+  // Update coro.begin instruction to refer to this constant
+  LLVMContext &C = F.getContext();
----------------
Period.

================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:313
@@ +312,3 @@
+
+  // we no longer need coro.end in F
+  removeCoroEnds(Shape);
----------------
Caps + period.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:215-217
@@ +214,5 @@
+          if (CoroSuspends.size() > 1) {
+            if (CoroSuspends.front()->isFinal())
+              report_fatal_error(
+                  "Only one suspend point can be marked as final");
+            std::swap(CoroSuspends.front(), CoroSuspends.back());
----------------
Do we have a noduplicate for this case?

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:243-244
@@ +242,4 @@
+            if (CoroEnds.front()->isFallthrough())
+              report_fatal_error(
+                  "Only one suspend point can be marked as final");
+            std::swap(CoroEnds.front(), CoroEnds.back());
----------------
Is this the right message?

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:263
@@ +262,3 @@
+
+    for (CoroSuspendInst *CS : CoroSuspends) {
+      auto CoroSave = CS->getCoroSave();
----------------
Maybe a comment here?

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:264
@@ +263,3 @@
+    for (CoroSuspendInst *CS : CoroSuspends) {
+      auto CoroSave = CS->getCoroSave();
+      CS->replaceAllUsesWith(UndefValue::get(CS->getType()));
----------------
I'd sink this into the if.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:264
@@ +263,3 @@
+    for (CoroSuspendInst *CS : CoroSuspends) {
+      auto CoroSave = CS->getCoroSave();
+      CS->replaceAllUsesWith(UndefValue::get(CS->getType()));
----------------
majnemer wrote:
> I'd sink this into the if.
`auto *`.

================
Comment at: lib/Transforms/Coroutines/Coroutines.cpp:273-277
@@ +272,7 @@
+    for (CoroEndInst *CE : CoroEnds) {
+      BasicBlock *BB = CE->getParent();
+      BB->splitBasicBlock(CE);
+      CE->eraseFromParent();
+      BB->getTerminator()->eraseFromParent();
+      new UnreachableInst(F.getContext(), BB);
+    }
----------------
I'd just call `changeToUnreachable` here.


https://reviews.llvm.org/D23461