[PATCH] D142620: [Coroutines] Improve rematerialization stage
Jannik Silvanus via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 31 00:41:13 PST 2023
jsilvanus 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;
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142620/new/
https://reviews.llvm.org/D142620
More information about the llvm-commits
mailing list