[TVM Discuss] [Development] Missing memoization in ExprFunctor
Currently I'm about to merge a PR https://github.com/apache/incubator-tvm/pull/5310 which adds memoization logic for `ExprFunctor` in two different derived classes. I've also noticed that there are many places in the code base that have the same duplicated memoization logic for `ExprFunctor`, e.g https://github.com/apache/incubator-tvm/blob/00014e20c3cc077727d467a67d3498260627e4e0/src/relay/backend/compile_engine.cc#L182-L191 https://github.com/apache/incubator-tvm/blob/00014e20c3cc077727d467a67d3498260627e4e0/src/relay/backend/graph_runtime_codegen.cc#L320 Since `ExprMutator` does have a memo built in, I wonder why `ExprFunctor` doesn't have it as well? I think we can clean up existing derived classes a lot. Is there a deep reason `ExprFunctor` cannot have a built in memoization? Since `ExprFunctor` doesn't have memo, derived classes need to override `VisitExpr(const Expr& expr)` method and implement dispatching logic as well, in addition to memo logic. I think it is a good item to add to our refactoring effort. cc @tqchen @zhiics --- [Visit Topic](https://discuss.tvm.ai/t/missing-memoization-in-exprfunctor/6334/1) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.ai/email/unsubscribe/e2fa975829637e4beff312a9d78930585064fe2950ed3a514467eacf3aa9f023).
[TVM Discuss] [Development] Missing memoization in ExprFunctor
ExprFunctor is a more generic base class for dispatching, while itself can be used to build recursive visitors, the visitor is not the only use case for the ExprFunctor. For example, we use the functor to build a dispatching Rewriter callbacks in the recent non-recursive vistor. Memoization could also have un-desired cosquences, if the translation depends on the context(e.g. if you are in the if and else branch, the results might be different). Due to these considerations, it is better to allow "no-surprise" and leave functors as simple as possible(thus no memoization) --- [Visit Topic](https://discuss.tvm.ai/t/missing-memoization-in-exprfunctor/6334/2) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.ai/email/unsubscribe/0ce4d8bc426f77d742394ef23a6f23d3c3efc54db3fc6ea47fea3bb874711ab0).
[TVM Discuss] [Development] Missing memoization in ExprFunctor
hmm, then does it make sense to have a another derived class, say `MemoizedExprFunctor`, that does have a memo logic built in, for a simple use case in derived classes that requires memoization? The PR I saw today could derive from this instead of ExprFunctor, for example. Basically for encapsulating already duplicated logics in our codebase. The rest of code that needs more care are not going to be affected. --- [Visit Topic](https://discuss.tvm.ai/t/missing-memoization-in-exprfunctor/6334/3) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.ai/email/unsubscribe/81affa1ec89201499c28a39a143dfd3f9b88261fa0c652e4dcf1938a4e3c72a2).
[TVM Discuss] [Development] Missing memoization in ExprFunctor
Given that the ExprFunctor also have additional arguments(besides the first argument), it is unclear how do we define such Memoization, unless we restrict the arguments to only contain the single one. The addtional cost for memo is not taking a lot of locs, so I think either way is fine(considering the tradeoff between reuse vs reduce the layer of abstractions) --- [Visit Topic](https://discuss.tvm.ai/t/missing-memoization-in-exprfunctor/6334/4) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.ai/email/unsubscribe/a01632345beed5e269861965dad36607e530cc26d1bb54eb9ccea080e8147640).
[TVM Discuss] [Development] Missing memoization in ExprFunctor
[quote="tqchen, post:4, topic:6334"] The addtional cost for memo is not taking a lot of locs [/quote] Note that in addition to the memo logic itself, we can also remove `VisitExpr(const Expr& expr)` override in the derived classes (which to me seems only exist for memoization), and hence the dispatching logic like this can be also removed: https://github.com/apache/incubator-tvm/blob/00014e20c3cc077727d467a67d3498260627e4e0/src/relay/backend/graph_runtime_codegen.cc#L322-L352 I think this dispatching logic is already handled by `ExprFunctor`. So if the base class has memo logic built in, dervied classes don't need to `VisitExpr(const Expr& expr)` override at all. The PR https://github.com/apache/incubator-tvm/pull/5310/ also has this dispatch logic in `VisitExpr(const Expr& expr)` override. Moreover, without memoization in the base class, all external codegen (most of which is not public and hence we don't know how many) needs to implement memoization in the codegen backend. I actually hit "exponential blow up" error when compiling resnet with my codegen because I was missing memoization. So I think reuse opportunity of memoization in the base class is high. [quote="tqchen, post:4, topic:6334"] Given that the ExprFunctor also have additional arguments(besides the first argument), it is unclear how do we define such Memoization, unless we restrict the arguments to only contain the single one. [/quote] It seems all use of `ExprFunctor` in the code base that adds memoization on top takes a single `const Expr&` argument, so I think single argument restriction is reasonable. So given these reason I think cost-to-benefit ratio of adding `MemoziedExprFunctor` is in favor. Anyway I'll send a PR and we can decide if it is a good addition. --- [Visit Topic](https://discuss.tvm.ai/t/missing-memoization-in-exprfunctor/6334/5) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.ai/email/unsubscribe/b19317774a274dce86a82992d7cacc10e7ea43a3c21bc9b26e308fe22543de27).
[TVM Discuss] [Development/RFC] [RFC] Visualizing Relay program as graph
Thanks for the great proposal. @hcho3, I followed your [script in
github](https://github.com/hcho3/relayviz) and noticed it missed a case when
operator being "Call"ed is a function.
Adding some code within "Call" condition can generate graph with each function
singled out.
```
elif isinstance(node, tvm.relay.expr.Call):
args = [node_dict[arg] for arg in node.args]
if isinstance(node.op, tvm.relay.Function):
print(f'node_idx: {node_idx},
Call(Function({node_dict[node.op.body]}))')
dot.node(str(node_idx),
f'Call(Function({node_dict[node.op.body]}))')
else:
print(f'node_idx: {node_idx}, Call(op_name={node.op.name},
args={args})')
dot.node(str(node_idx), f'Call(op={node.op.name})')
for arg in args:
dot.edge(str(arg), str(node_idx))
elif isinstance(node, tvm.relay.Function):
print(f'node_idx: {node_idx},
Function(body={node_dict[node.body]})')
dot.node(str(node_idx), f'Function'+str({node_dict[node.body]}))
dot.edge(str(node_dict[node.body]), str(node_idx))
```
---
[Visit
Topic](https://discuss.tvm.ai/t/rfc-visualizing-relay-program-as-graph/4825/15)
to respond.
You are receiving this because you enabled mailing list mode.
To unsubscribe from these emails, [click
here](https://discuss.tvm.ai/email/unsubscribe/7bcc544c68d3e2a08eb015089fecb9db2bf1122c769665014c89432917f27519).
