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 2021/02/10 17:28:23 UTC
[GitHub] [tvm] trevor-m opened a new pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
trevor-m opened a new pull request #7436:
URL: https://github.com/apache/tvm/pull/7436
After https://github.com/apache/tvm/pull/7417 the cmake rules for TensorRT and ACL BYOC would cause the runtime sources to be compiled twice if both runtime and codegen are enabled together, giving the following error:
```
MakeFiles/tvm_runtime_objs.dir/src/runtime/contrib/tensorrt/tensorrt_runtime.cc.o: In function `tvm::runtime::contrib::TensorRTRuntimeCreate(tvm::runtime::String const&, tvm::runtime::String const&, tvm::runtime::Array<tvm::runtime::String, void> const&)':
tensorrt_runtime.cc:(.text+0x4e0): multiple definition of `tvm::runtime::contrib::TensorRTRuntimeCreate(tvm::runtime::String const&, tvm::runtime::String const&, tvm::runtime::Array<tvm::runtime::String, void> const&)'
CMakeFiles/tvm_objs.dir/src/runtime/contrib/tensorrt/tensorrt_runtime.cc.o:tensorrt_runtime.cc:(.text+0x4e0): first defined here
collect2: error: ld returned 1 exit status
CMakeFiles/tvm.dir/build.make:842: recipe for target 'libtvm.so' failed
make[2]: *** [libtvm.so] Error 1
CMakeFiles/Makefile2:131: recipe for target 'CMakeFiles/tvm.dir/all' failed
make[1]: *** [CMakeFiles/tvm.dir/all] Error 2
Makefile:148: recipe for target 'all' failed
make: *** [all] Error 2
```
@lhutton1
----------------------------------------------------------------
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] [tvm] comaniac merged pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #7436:
URL: https://github.com/apache/tvm/pull/7436
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r573956757
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
During codegen, we still need the source file where the custom runtime module is defined so that it can 1) be created and 2) SaveToBinary can be called on it.
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/tensorrt/codegen.cc#L208
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/arm_compute_lib/codegen.cc#L370
We could add the files to runtime instead, I don't think it would make a difference.
----------------------------------------------------------------
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] [tvm] tkonolige commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r573937245
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
I'm a little confused here. If these are for the runtime, why are they being included in the compiler sources?
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574065135
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
I see, I think that accomplishes the exact same thing as this change, but instead of conditionally adding tensorrt_runtime.cc in USE_TERSORRT_CODEGEN, it would be conditionally excluding it in USE_TENSORRT_RUNTIME if its already included by USE_TERSORRT_CODEGEN
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r573956757
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
During codegen, a runtime module needs to be created, so we still need the source file where the custom runtime module is defined so that it can 1) be created and 2) SaveToBinary can be called on it.
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/tensorrt/codegen.cc#L208
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/arm_compute_lib/codegen.cc#L370
We could add the files to runtime instead, I don't think it would make a difference.
----------------------------------------------------------------
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] [tvm] comaniac commented on pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#issuecomment-776882300
Also cc @tkonolige
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574051855
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
Not sure what difference using glob or not would make since only `tensorrt_runtime.cc` is added
----------------------------------------------------------------
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] [tvm] tkonolige commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574045801
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
What if we just don't use GLOB here?
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r573956757
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
During codegen, we still need the source file where the custom runtime module is defined so that it can 1) be created and 2) SaveToBinary can be called on it.
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/tensorrt/codegen.cc#L208
https://github.com/apache/tvm/blob/main/src/relay/backend/contrib/arm_compute_lib/codegen.cc#L370
We could add the files to runtime srcs instead, I don't think it would make a difference.
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574051332
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
It needs to be possible to compile with `USE_TENSORRT_CODEGEN` enabled only (and not runtime).
----------------------------------------------------------------
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] [tvm] trevor-m commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574065135
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
We need to include tensorrt_runtime.cc though for `USE_TENSORRT_CODEGEN`
----------------------------------------------------------------
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] [tvm] tkonolige commented on a change in pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#discussion_r574054099
##########
File path: cmake/modules/contrib/TensorRT.cmake
##########
@@ -28,7 +28,9 @@ if(USE_TENSORRT_CODEGEN)
file(GLOB RUNTIME_TENSORRT_SRCS src/runtime/contrib/tensorrt/tensorrt_runtime.cc)
set_source_files_properties(${RUNTIME_TENSORRT_SRCS} PROPERTIES COMPILE_FLAGS "-Wno-deprecated-declarations")
list(APPEND COMPILER_SRCS ${COMPILER_TENSORRT_SRCS})
- list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
+ if(NOT USE_TENSORRT_RUNTIME)
+ list(APPEND COMPILER_SRCS ${RUNTIME_TENSORRT_SRCS})
Review comment:
In the `if(USE_TENSORRT_RUNTIME)` we could not use GLOB and explicitly list the files and not include tensors_runtime.cc. Then we wouldn't need to do the conditional.
But, if USE_TERSORRT_CODEGEN can be disabled while USE_TENSORRT_RUNTIME is enabled, then the current approach is fine.
----------------------------------------------------------------
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] [tvm] comaniac commented on pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#issuecomment-777851731
Thanks @trevor-m @tkonolige @lhutton1
----------------------------------------------------------------
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] [tvm] codeislife99 commented on pull request #7436: [CMAKE] Fix double compile of runtime sources for TRT, ACL
Posted by GitBox <gi...@apache.org>.
codeislife99 commented on pull request #7436:
URL: https://github.com/apache/tvm/pull/7436#issuecomment-776972280
Thanks Trevor.
----------------------------------------------------------------
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