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

[witstorm via Phabricator via llvm-commits]

witstorm95 added a comment.

@MatzeB I'm sorry it took so long to get back to you as I have no time to do this.

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.

If we want to remove these redundant operation, there still are ways to do it. I means we can do it based on your previous patch(https://reviews.llvm.org/D156835 <https://reviews.llvm.org/D156835>). I will give your some advises to improve on your patch. Let we jump to D156835 <https://reviews.llvm.org/D156835>.



================
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;
----------------
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.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:235-236
 
     // We don't need to count the predecessors when initialization.
     if constexpr (!Initialize)
       // If all the predecessors of the current Block don't change,
----------------
MatzeB wrote:
> Well we should definitely check the predecessors even in Initialization now, in an RPO order we should have nearly all of them computed already (everything except for backedges).
Every block has marked `Changed ` as true before initial iteration. So we don't need to check if the predecessors has changed(Always changed) in initialization.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:239-244
       if (all_of(predecessors(B), [this](BasicBlock *BB) {
             return !Block[Mapping.blockToIndex(BB)].Changed;
           })) {
         B.Changed = false;
         continue;
       }
----------------
MatzeB wrote:
> Oh guess `B.Changed` is read here. But how does this work? Isn't `Changed` always `false` after the `Initialize == true` round? Wouldn't we just skip processing any block in the 2nd round then because of this?
In the initial iteration(`Initialize == true`), we will not update `Changed`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156850