[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.

> For code reuse, I don't really think split computeBlockData to 2 functions is good idea.

My major concern is about the readability. The so called 1,2,3 iterations are confusing. It is magic numbers and reader may have a hard time to read them.

Then out of curiosity, are the algorithm capable to handle the following case?

  J -> I
  I -> S
  S -> I

S is a suspend block. And `I.Kill[J]` should be false since the path from `J` to `I` that contains the suspend block must repeat node I. So it doesn't fit into the definition of `Kill`. I feel the algorithm doesn't get this right but I am super sure. It is better to give it a test case too.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:124
+  /// As we all know that topological sorting need to be used on DAG. But CFG
+  /// maybe not a DAG as CFG may have loop, so we need to break the loop.
+  /// If current CFG is a DAG, we just need to visit once. If current CFG is not
----------------
witstorm95 wrote:
> ChuanqiXu wrote:
> > In the algorithm, I prefer to use the term `back edge` than `loop`. Since loop has many more meanings and attributes so that the back edges would be more precise.
> > 
> > Also with the new term `back edge`, we can describe the algorithm much more simpler:
> > 1. In the initial iterations, all the consume information along the forward edges are collected.
> > 2. (If there is any back edges), we can iterate again to get all the consume information along the back edges.
> > 3. We can compute the Kill informations by the consume informations.
> > 
> > In this way, it is much easier to understand too.
> @ChuanqiXu Thanks for your comment. 
> 
> I takes some time to figure out what means about terms like back edge and forward edge. Here is [[ https://courses.csail.mit.edu/6.006/fall11/rec/rec14.pdf | DFS Edge Classification]]. It's really suitable for topological sorting algorithm?
> 
> If we use that tems, I have some questions about your description of this algorithm:
> 
> > In the initial iterations, all the consume information along the forward edges are collected.
> The Consume info should along the tree edge and back edge(if exists) are collected. And the same time, the Kill info should be collected too as we don't know there exists back edges or not in initial iteration.
> 
> 
> > (If there is any back edges), we can iterate again to get all the consume information along the back edges.
> 
> The Consume info should along the tree edge and back edge are collected again and the Kill info should be collected too as you can't compute the all Kill informations in the third iteration only.
> 
> 
> I takes some time to figure out what means about terms like back edge and forward edge. Here is DFS Edge Classification. It's really suitable for topological sorting algorithm?

Sorry for that it is confusing. This is a suggestion instead of a requirement. Different areas have different conventions for terminologies. For example, I didn't see the definition of tree edge and cross edge in the link you give before.

In compiler engineerings, the term 'loop' has many more meanings than in pure algorithms. So that the instinct reaction that I had when I see your patch was that you did something wrong. Since I felt if you want loop informations, you should use the existing components to query the loop information. But then I realized all you need about is the back edges.

> you can't compute the all Kill informations in the third iteration only.

It is the third step and it is not necessarily an iteration. For example, in your current implementations, it may still access a node multiple times if it has a lot of predecessors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154695