-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay] C++ GraphRuntimeCodegen, Deprecate Python2 #2986
Conversation
231c265
to
e570ac1
Compare
a5d19fc
to
e005ae1
Compare
/*! \brief Visitors */ | ||
std::unordered_map<Expr, std::vector<GraphNodeRef>, NodeHash, NodeEqual> visitor_cache_; | ||
|
||
std::vector<GraphNodeRef> VisitExpr(const Expr& expr) override { |
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.
Do we need to overload this function?, this should happen by default
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.
I think we need if we want to use cache.
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.
Oh right caching only exists for the Mutator/Visitor
CompileEngine compile_engine_; | ||
}; | ||
|
||
class GraphRuntimeCodegenModule : public runtime::ModuleNode { |
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.
I think this makes sense for now, but in general I'm not a huge fan of reusing tvm::Module to implement this stringly typed function lookup map. Is there a better way to do this @tqchen ?
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.
It is a stateful function. I think Module is a convenient & clean way to do this.
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.
Yeah I understand, the string based dispatch of module isn't my favorite design we just did the same thing in the VTA simulator stuff.
Just some minor comments from me. |
177b823
to
01abcaf
Compare
@jroesch Relay C++ Build module depends on this PR. Please let me know if there is anything more needed for this PR. |
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.
Nice change! Have a minor comment.
@antinucleon can you just fix the last comment from @wweic then we can merge? |
Thanks @antinucleon @jroesch @wweic, this is now merged. |
* [Relay] C++ GraphRuntimeCodegen * [Test] Deprecate Python2 * [Python3] Add Py2 check * Update _pyversion.py * [Python3] Update test
* [Relay] C++ GraphRuntimeCodegen * [Test] Deprecate Python2 * [Python3] Add Py2 check * Update _pyversion.py * [Python3] Update test
Add C++ GraphRuntimeCodegen.
Deprecate Python GraphRuntimeCodegen.
Deprecate Python2.