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/01/16 02:52:54 UTC

[GitHub] [incubator-tvm] hlu1 opened a new pull request #4723: [Relay] Port relay.backend.build to c++

hlu1 opened a new pull request #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723
 
 
   `relay.backend.build` is implemented in python and is used in C++ in https://github.com/apache/incubator-tvm/blob/c69092ae0d39f9a5161f098d933d0a2ec570a2c5/src/relay/backend/interpreter.cc#L422 and https://github.com/apache/incubator-tvm/blob/ce807fe83825bce666ed1834ab24b5d6ddfa6bca/src/relay/backend/compile_engine.cc#L602
   
   Porting it to C++ allows to compile a relay graph e2e in C++.
   
   @jroesch, @MarisaKirisame please review :)  

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575251726
 
 
   @hlu1 Can we simply remove this packed function and directly invoke tvm::build from compile_engine and interpreter? We have two separate pipelines from Python and C++. I think we can probably directly use tvm.build from python and tvm::build from C++. 

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] MarisaKirisame commented on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575048901
 
 
   I know very little about tvm Module and relay build, so I might get something wrong.
   But it seems like you replace 27 line with 47 line, but they are implemented completely differently - one call another function while another does lots of work. Any reason for this?

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter
URL: https://github.com/apache/incubator-tvm/pull/4723#discussion_r367756167
 
 

 ##########
 File path: src/relay/backend/compile_engine.cc
 ##########
 @@ -599,12 +599,13 @@ class CompileEngineImpl : public CompileEngineNode {
     CCacheValue value = LowerInternal(key);
     if (value->packed_func != nullptr) return value->packed_func;
     // build the function.
+    tvm::runtime::Module m;
     if (const auto* f = runtime::Registry::Get("relay.backend.build")) {
-      tvm::runtime::Module m = (*f)(value->cached_func->funcs, key->target);
-      value->packed_func = m.GetFunction(value->cached_func->func_name);
+      m = (*f)(value->cached_func->funcs, key->target);
     } else {
-      LOG(FATAL) << "relay.backend.build is not registered";
+      m = build(value->cached_func->funcs, key->target, key->target, BuildConfig::Current());
 
 Review comment:
   according to the syntax we had in Python, should we do the following?
   
   ```c++
   m = build(value->cached_func->funcs, key->target, Target(nullptr), BuildConfig::Current());
   ```
   Same to the one in intepreter.cc

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] hlu1 commented on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
hlu1 commented on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575365298
 
 
   @zhiics, makes sense. Will do. 

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575247517
 
 
   cc @zhiics @jroesch please also help to take a look

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics merged pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter

Posted by GitBox <gi...@apache.org>.
zhiics merged pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter
URL: https://github.com/apache/incubator-tvm/pull/4723
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics edited a comment on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575251726
 
 
   @hlu1 Can we simply remove this packed function and directly invoke tvm::build from compile_engine and interpreter? We have two separate pipelines from Python and C++. I think we can probably directly use tvm.build from python and tvm::build from C++. So we don't really need to interoperate between them.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] hlu1 commented on issue #4723: [Relay] Port relay.backend.build to c++

Posted by GitBox <gi...@apache.org>.
hlu1 commented on issue #4723: [Relay] Port relay.backend.build to c++
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575019899
 
 
   Sorry I missed a few things at the beginning and just added them.
   
   Do you mean translating this part line by line, https://github.com/apache/incubator-tvm/blob/master/python/tvm/build_module.py#L509-L650?
   
   Some of the type checks are not needed in C++, and the signature of https://github.com/apache/incubator-tvm/blob/bfb4884e47f7d2c759b2ee707aa18acf35116380/python/tvm/relay/backend/_backend.py#L65-L88 is a lot simpler.
   
   Also, 
   https://github.com/apache/incubator-tvm/blob/master/python/tvm/build_module.py#L635-L650 has been implemented in here https://github.com/apache/incubator-tvm/blob/master/src/codegen/build_module.cc#L278-L319. All I need to do is call this function.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] hlu1 commented on a change in pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter

Posted by GitBox <gi...@apache.org>.
hlu1 commented on a change in pull request #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter
URL: https://github.com/apache/incubator-tvm/pull/4723#discussion_r367793036
 
 

 ##########
 File path: src/relay/backend/compile_engine.cc
 ##########
 @@ -599,12 +599,13 @@ class CompileEngineImpl : public CompileEngineNode {
     CCacheValue value = LowerInternal(key);
     if (value->packed_func != nullptr) return value->packed_func;
     // build the function.
+    tvm::runtime::Module m;
     if (const auto* f = runtime::Registry::Get("relay.backend.build")) {
-      tvm::runtime::Module m = (*f)(value->cached_func->funcs, key->target);
-      value->packed_func = m.GetFunction(value->cached_func->func_name);
+      m = (*f)(value->cached_func->funcs, key->target);
     } else {
-      LOG(FATAL) << "relay.backend.build is not registered";
+      m = build(value->cached_func->funcs, key->target, key->target, BuildConfig::Current());
 
 Review comment:
   Fixed

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on issue #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter

Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #4723: [Relay] Invoke tvm::build from relay compile_engine and interpreter
URL: https://github.com/apache/incubator-tvm/pull/4723#issuecomment-575709489
 
 
   Thanks @hlu1 @MarisaKirisame 

----------------------------------------------------------------
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


With regards,
Apache Git Services