[PATCH] D23586: [Coroutines] Part 8: Coroutine Frame Building algorithm

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:402-403
@@ +401,4 @@
+  for (auto const &E : Spills) {
+    // If we have not seen the value, generate a spill.
+    if (CurrentValue != E.def()) {
+      CurrentValue = E.def();
----------------
Maybe:
  if (CurrentValue == E.def())
    continue;

This will let you reduce the indentation.

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:483-484
@@ +482,4 @@
+
+  SmallVector<BasicBlock *, 8> Preds(pred_begin(&BB), pred_end(&BB));
+  for (BasicBlock *Pred : Preds) {
+    auto *IncomingBB = SplitEdge(Pred, &BB);
----------------
What prevents you from visiting the same pred twice?

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:487-488
@@ +486,4 @@
+    IncomingBB->setName(BB.getName() + Twine(".from.") + Pred->getName());
+    auto *PN = cast<PHINode>(&BB.front());
+    do {
+      int Index = PN->getBasicBlockIndex(IncomingBB);
----------------
I think you could structure this like a normal for loop:
  for (auto *PN = cast<PHINode>(&BB.front()); PN != nullptr; PN = dyn_cast<PHINode>(PN->getNextNode()))

================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:596-597
@@ +595,4 @@
+
+  // Put final CoroEnd into its own block.
+  splitAround(Shape.CoroEnds.front(), "CoroEnd");
+
----------------
The comment says "final" but you are accessing .front().


Repository:
  rL LLVM

https://reviews.llvm.org/D23586