Conversation
…to recursive function calls.
|
CC @rkimball - given that this affects windows support |
|
Also CC @tkonolige |
|
Is it possible to use the MixedModeVisitor instead of manually creating an imperative stack here? cc @mbs-octoml |
Thanks for the pointer to MixedModeVisitor. It appears to me that MixedModeVisitor does something like: The device_planner does something like: |
|
cc @mbs-octoml |
| // We don't care if the call is in pre- or post-lowered form. | ||
| auto vanilla_call = GetAnyCall(call_node); | ||
|
|
||
| // Find the higher-order domain for the callee. See DomainForCallee for the special rules |
There was a problem hiding this comment.
Can you please preserve the comments? Thanks!
| << domains_->ToString(func_domain) << std::endl | ||
| << "and implied call virtual devices:" << std::endl | ||
| << domains_->ToString(implied_domain); | ||
| std::stack<Expr> stack; |
There was a problem hiding this comment.
I wrote this as a "naive" ExprVisitor and not as a MixedModeVisitor since I wrongly assumed it would only see post-ANF graphs. Hence the iterative rather than recursive handling of LetNodes but arbitrary recursion everywhere else. So sorry this is causing you trouble!
Since it is just setting up domain constraints the order of visiting shouldn't matter much (at least for valid Relay graphs) and there's no tricky state management inside the visitor. So I think this could be converted to a MixedModeVisitor pretty easily. Would you be comfortable trying to do that? Happy to help review that, offer help, etc.
There was a problem hiding this comment.
Do you have time to rewrite this pass as MixedModeVisitor? I am time-stretched,
There was a problem hiding this comment.
I opened a bug: #10135
Could you help to fix it? I can help to test the fix on windows.
There was a problem hiding this comment.
I can also do it when I have time.
…ows due to recursive function calls." This reverts commit 7058136.
…to recursive function calls.
@masahi
Stack overflow are observed when large models are compiled on windows. This is due to recursive function calls in many places. This patch fixes one such place. A test is added to generate a big enough graph to expose this problem.