[PATCH] D154695: [Coroutines] Add an O(n) algorithm for computing the cross suspend point information.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

BTW, I feel the grammar of the comment reads slightly bad. Could you try to improve it? I can't help a lot since I am not native speaker.

> As you can see, some coro tests failed. Do you know the problem ?

I don't have an idea. Folly is pretty complex. Maybe you can try to choose another stable version for folly.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:43
 #include <optional>
+#include <unordered_set>
 
----------------
Generally we prefer llvm::DenseSet instead of unordered_set. You can refer to the LLVM Programmers manual.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:150
+
+  /// Returns true if there exists back edges in CFG.
+  template <bool HasBackEdge = false>
----------------
It is better to explain the meaning of the parameters too.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:153
+  bool collectConsumeKillInfo(size_t EntryNo,
+                              SmallVector<int> const &BlocksIndegree);
 
----------------
nit: we generally prefer the style.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:264
+    size_t EntryNo, SmallVector<int> const &BlocksIndegree) {
+  bool ExistBackEdge = false;
+  // Copy BlocksIndegree to IndegreeOfBlocks.
----------------
nit:


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:265
+  bool ExistBackEdge = false;
+  // Copy BlocksIndegree to IndegreeOfBlocks.
+  auto IndegreeOfBlocks = BlocksIndegree;
----------------
Such comment is meaningless.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:267
+  auto IndegreeOfBlocks = BlocksIndegree;
+  // Block CandidateQueue with indegree zero.
+  std::queue<size_t> CandidateQueue;
----------------
So it is better to explain what's the meaning for *indegree*. I know the definition of in degree. But I feel it has different semantics in this algorithm. For example, what's the information represented by `indegree 0`? Why the indegree of a node change while we don't touch the graph? So I think you'd better to give it a different name and a corresponding explanation.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:271-288
+  // Visit I.
+  auto visited = [&](size_t I) {
+    switch (IndegreeOfBlocks[I]) {
+    case 0:
+      break;
+    case 1: {
+      CandidateQueue.push(I);
----------------
Same with above.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:290
 
-    if constexpr (!Initialize) {
-      B.Changed = (B.Kills != SavedKills) || (B.Consumes != SavedConsumes);
-      Changed |= B.Changed;
+  // Push EntryNo into CandidateQueue.
+  CandidateQueue.push(EntryNo);
----------------
This is pretty clear. We don't need such comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:329
+    // there exists back edge and need to break it.
+    if (CandidateQueue.empty() && MaybeBackEdgeSet.size()) {
+      ExistBackEdge = true;
----------------
nit


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:366
 
+  // Set EntryNo.
+  size_t EntryNo = Mapping.blockToIndex(&(F.getEntryBlock()));
----------------



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:404
+  if (collectConsumeKillInfo(EntryNo, BlocksIndegree))
+    collectConsumeKillInfo<true>(EntryNo, BlocksIndegree);
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154695