You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/06/13 07:32:10 UTC
[GitHub] [incubator-tvm] lixiaoquan opened a new pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
lixiaoquan opened a new pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797
A function may refer to other resources in the same module, so keep
the content of original module when infering a function.
cc @mbaret @mbrookhart @comaniac
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#discussion_r440504192
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -36,22 +36,24 @@ namespace tvm {
namespace relay {
namespace merge_composite {
-Function InferType(const Function& expr) {
- auto mod = IRModule::FromExpr(expr);
+Function InferType(const Function& expr, const IRModule& m) {
+ IRModule mod(m);
+ mod->Update(mod->GetGlobalVar("main"), expr);
Review comment:
Or maybe we can rewrite MergeComposite to be a module pass so that we can iterate `functions` by ourselves.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-644515613
PyTorch frontend also uses Prelude. It contains List ADT (as in functional programming) and functions on lists (cons, map, filter, concat, length, nth etc).
It allows creating and manipulating dynamic lists. This is necessary for supporting python list append, for example.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-645038057
Thanks @lixiaoquan @comaniac @mbrookhart
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#discussion_r440335267
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -36,22 +36,24 @@ namespace tvm {
namespace relay {
namespace merge_composite {
-Function InferType(const Function& expr) {
- auto mod = IRModule::FromExpr(expr);
+Function InferType(const Function& expr, const IRModule& m) {
+ IRModule mod(m);
+ mod->Update(mod->GetGlobalVar("main"), expr);
Review comment:
Since now we update the original module with new transformed function, should we update the corresponding function instead of `main`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi edited a comment on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-644515613
PyTorch frontend also uses Prelude. It contains List ADT (as in functional programming) and functions on lists (cons, map, filter, concat, length, nth etc).
It allows creating and manipulating dynamic lists. For pytorch frontend, this is necessary for supporting python list append, for example.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] lixiaoquan commented on a change in pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#discussion_r440584386
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -36,22 +36,24 @@ namespace tvm {
namespace relay {
namespace merge_composite {
-Function InferType(const Function& expr) {
- auto mod = IRModule::FromExpr(expr);
+Function InferType(const Function& expr, const IRModule& m) {
+ IRModule mod(m);
+ mod->Update(mod->GetGlobalVar("main"), expr);
Review comment:
> Since now we update the original module with new transformed function, should we update the corresponding function instead of `main`?
For now, other functions in module don't call 'main', so it is safe to replace 'main'. If we are infering function which is mutated from 'main', that's just what we want, if we are infering other functions, it will be a duplicated function but it doesn't harm. And, with only a mutated function, it seems we can't find it in original global_var_map_ because we don't know its crossponding global variable's name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi edited a comment on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-644515613
PyTorch frontend also uses Prelude. It contains List ADT (as in functional programming) and functions on lists (cons, map, filter, concat, length, nth etc).
It allows creating and manipulating dynamic lists. For pytorch frontend, this is necessary for supporting python list append, for example.
Prelude is mostly rewritten in the Relay "language" (file extension with .rly). Relay has a parser for that language which translate text rep to C++ rep, also usable from python.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi edited a comment on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-644515613
PyTorch frontend also uses Prelude. It contains List ADT (as in functional programming) and functions on lists (cons, map, filter, concat, length, nth etc).
It allows creating and manipulating dynamic lists. For pytorch frontend, this is necessary for supporting python list append, stacking a list of tensors, for example.
Prelude is mostly rewritten in the Relay "language" (file extension with .rly). Relay has a parser for that language which translate text rep to C++ rep, also usable from python.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#discussion_r440993336
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -36,22 +36,24 @@ namespace tvm {
namespace relay {
namespace merge_composite {
-Function InferType(const Function& expr) {
- auto mod = IRModule::FromExpr(expr);
+Function InferType(const Function& expr, const IRModule& m) {
+ IRModule mod(m);
+ mod->Update(mod->GetGlobalVar("main"), expr);
Review comment:
Make sense, although I still prefer to avoid dummy modules. I think it's fine to let his PR in first, and maybe I can fix that later on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi merged pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] lixiaoquan commented on pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#issuecomment-644477410
> LGTM
>
> For my edification, is there a tutorial on what Prelude actually does somewhere? It's not something I've been able to fit into my mental model of TVM yet.
TensorFlow frontend generates a module with Prelude.
https://github.com/apache/incubator-tvm/blob/7a419718c121164fc260864014e1d0d81f556949/python/tvm/relay/frontend/tensorflow.py#L2729
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on a change in pull request #5797: [MergeComposite] Fix InferType when module contains Prelude
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #5797:
URL: https://github.com/apache/incubator-tvm/pull/5797#discussion_r440502738
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -36,22 +36,24 @@ namespace tvm {
namespace relay {
namespace merge_composite {
-Function InferType(const Function& expr) {
- auto mod = IRModule::FromExpr(expr);
+Function InferType(const Function& expr, const IRModule& m) {
+ IRModule mod(m);
+ mod->Update(mod->GetGlobalVar("main"), expr);
Review comment:
mmm, you're right, reading the pass infrastructure a little more today.
The FunctionPass, however, doesn't seem to pass the information on what Function we're see down to the passes: https://github.com/apache/incubator-tvm/blob/8578096853eec5711bfcc9a3a68145fd12a135cb/src/relay/ir/transform.cc#L123-L132
I guess we can either change that API (which touches a lot of passes), or maybe invert this Map https://github.com/apache/incubator-tvm/blob/4347b41a5e64a2a453297b371232d6e101051b3c/include/tvm/ir/module.h#L53, find the global var, and store that in the class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org