[PATCH] D156850: [NFC][Coroutines] Use a reverse post-order to guide the computation about cross suspend infomation to reach a fixed point faster.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.

> As you can see, there still has many redundant operations. Just like the last iteration is to check whether it has reached a fixed point and etc.

Yes, but it is simple and fast enough. Hence I abandoned my other patch and hoped we can go just go with this. It nicely fixes the original issue and similar programs.

While a solution based on a priority queue should be better when programs become arbitrarily large, I'm not convinced the added complexity will help common "normal" sized functions and my main worry is that it adds more complexity IMO.

LGTM from me on this patch.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:227-228
 
-template <bool Initialize> bool SuspendCrossingInfo::computeBlockData() {
-  const size_t N = Mapping.size();
+template <bool Initialize, class BBRangeTy>
+bool SuspendCrossingInfo::computeBlockData(const BBRangeTy &BBRange) {
   bool Changed = false;
----------------
witstorm95 wrote:
> MatzeB wrote:
> > Just put `ReversePostOrderTraversal<Function *>&` here. Adding a template parameter that only ever gets the same type just adds needless complexity.
> I guess use BBRangeTy just want to make computeBlockData more general. It means you can specify another BBRange to guide the computation and no need to modificy here again.
Yes, it is more general. But more general is not automatically better especially if the chance of there ever being another type is really low. You have to consider that what mostly happens to code is that it is read by other people! What happens less often is that the code is written and even less often that the type is changed. In this case I think a fixed type helps readability (at the tiny cost of having to type more when we actually want to change the type in the future).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156850