New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bugfix][Module] Fix recursive GetFunction in runtime::Module #6859
[Bugfix][Module] Fix recursive GetFunction in runtime::Module #6859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for getting this fixed!
@@ -68,6 +68,9 @@ PackedFunc ModuleNode::GetFunction(const std::string& name, bool query_imports) | |||
if (query_imports) { | |||
for (Module& m : self->imports_) { | |||
pf = m.operator->()->GetFunction(name, query_imports); | |||
if (pf != nullptr) { | |||
return pf; | |||
} | |||
} | |||
} | |||
return pf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth (and correct) putting an assert like ICHECK(pf != nullptr) << "unable to get the function pointer"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected behvaior of this function is to return nullptr for functions inavailable, so we don't want to put ICHECKhere
Thanks @junrushao1994 ! Please also send another PR to https://github.com/apache/incubator-tvm/tree/v0.7 branch |
This PR fixes a bug that affects the behavior of
ModuleNode::GetFunction
whenquery_import=True
CC: @jwfromm @tqchen @icemelon9