[PATCH] D142620: [Coroutines] Improve rematerialization stage

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2248
+    RematGraph *RG = E.second.get();
+    ReversePostOrderTraversal<RematGraph *> RPOT(RG);
+    using rpo_iterator = ReversePostOrderTraversal<RematGraph *>::rpo_iterator;
----------------
jsilvanus wrote:
> ChuanqiXu wrote:
> > It is expensive to create ReversePostOrderTraversal. So it looks not good to construct it in a loop.
> Pedantically speaking, I'm not sure constructing the `ReversePostOrderTraversal` in a loop here is an issue: It being "expensive" just means it does the graph traversal in the constructor, so its run time is linear in the size of the graph.
> But here we are using it to traverse *different* graphs, all of which have been constructed before, so the runtime can be amortized into the construction of those graphs, or also into the traversal that is done later.
> 
> What we should not do is re-creating `ReversePostOrderTraversal` iterator objects for the same graph in a loop, because that wastes runtime.
> 
> Still, one might argue that constructing all those graphs with overlapping nodes, i.e. possibly multiple graphs having a node for the same `Instruction*`, is a fundamental runtime issue. Not sure if that really can become an issue?
Yeah, the key point here is that how many overlapping nodes there is. Have you measured the compile-time, run time performance or memory usages? Then we can have a better feeling. For example, we can decide if we want to limit the depth of the graph then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142620