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 2022/06/08 22:56:42 UTC
[GitHub] [tvm] csullivan opened a new pull request, #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
csullivan opened a new pull request, #11635:
URL: https://github.com/apache/tvm/pull/11635
Only include Hexagon runtime sources when building for hexagon rpc on hardware and do not include them
for x86 (host, simulator) or android builds.
Duplicate of #11595.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r893511615
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
When would `BUILD_FOR_HEXAGON` be false, but `USE_HEXAGON_RPC` be `"HEXAGON"`? It seems like the latter implies the former...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r893873916
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
What I mean is that it seems that `(${USE_HEXAGON_RPC} == "HEXAGON") => BUILD_FOR_HEXAGON`. Unless I'm missing something, you can just remove `USE_HEXAGON_RPC` from x86/android builds (and also from this check).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r893873916
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
What I mean is that it seems that `(${USE_HEXAGON_RPC} == "HEXAGON") => BUILD_FOR_HEXAGON`. Unless I'm missing something, you can just remove `USE_HEXAGON_RPC` from x86/android builds.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] adstraw commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r894914002
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
I tested with `if(BUILD_FOR_HEXAGON)` and it appears to work at least for PR #11653 which depends on this PR to pass CI.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] adstraw commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r894932083
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
I am getting some strange results if I fail to set `USE_HEXAGON_RPC` for the x86/Android builds. For Android, there is a linker error. For x86, some sort of runtime simulation error. I did not spend time to debug these errors.
However, I can revert the changes to `hexagon_api/CMakeLists.txt` and just set `USE_HEXAGON_RPC=ON` for all three builds while checking for `if(BUILD_FOR_HEXAGON)` in Hexagon.cmake and it appears to work.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] adstraw commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r894936088
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
We may be able to "remove `USE_HEXAGON_RPC` from x86/android builds" in `hexagon_api/CMakeLists.txt` as @kparzysz-quic but I am getting some strange results when doing so. I reverted the changes to that file in this PR.
Changing to `if(BUILD_FOR_HEXAGON)` and removing the API check works and accomplishes the original intent of the PR.
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
We may be able to "remove `USE_HEXAGON_RPC` from x86/android builds" in `hexagon_api/CMakeLists.txt` as @kparzysz-quic suggests but I am getting some strange results when doing so. I reverted the changes to that file in this PR.
Changing to `if(BUILD_FOR_HEXAGON)` and removing the API check works and accomplishes the original intent of the PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r893864179
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
BUILD_FOR_HEXAGON is false for the runtime android and x86 build targeted by the hexagon_api, and USE_HEXAGON_RPC is true for these cases. Thus I want to tighten the requirement so that hexagon sources are not brought in to the tvm_runtime builds (initiated by the hexagon_api cmake) when we aren't building for hexagon.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] adstraw commented on a diff in pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #11635:
URL: https://github.com/apache/tvm/pull/11635#discussion_r894932083
##########
cmake/modules/Hexagon.cmake:
##########
@@ -116,7 +116,7 @@ function(add_hexagon_wrapper_paths)
link_directories("${HEXAGON_TOOLCHAIN}/lib/iss")
endfunction()
-if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
+if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Review Comment:
I am getting some strange results if I fail to set `USE_HEXAGON_RPC` for the x86/Android builds. For Android, there is a linker error. For x86, some sort of runtime simulation error. I did not spend time to debug these errors.
However, I can revert the changes to `hexagon_api/CMakeLists.txt` and just set `USE_HEXAGON_RPC=ON` for all three builds while checking for `if(BUILD_FOR_HEXAGON)` in Hexagon.cmake and it appears to work.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic merged pull request #11635: [Hexagon] Tighten requirements on inclusion of runtime sources
Posted by GitBox <gi...@apache.org>.
kparzysz-quic merged PR #11635:
URL: https://github.com/apache/tvm/pull/11635
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org