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/04/19 23:41:27 UTC
[GitHub] [tvm] areusch commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
areusch commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r853581856
##########
python/tvm/contrib/hexagon/build.py:
##########
@@ -182,7 +182,7 @@ def upload(self, local_path: Union[str, pathlib.Path], remote_filename: str):
assert self._workspace
self._copy_to_remote(local_path, os.path.join(str(self._workspace), remote_filename))
- def start_session(self) -> Session:
+ def start_session(self, name="hexagon-rpc") -> Session:
Review Comment:
nit: keep the kwarg named the same so it's easier to analyze as we move up and down the stack. also, can you update the docstring?
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -60,10 +60,10 @@ def __init__(
rpc_receive_buffer_size_bytes: int = 2 * 1024 * 1024,
):
self._launcher = launcher
- self._session_name = session_name
- self._remote_stack_size_bytes = remote_stack_size_bytes
- self._rpc_receive_buffer_size_bytes = rpc_receive_buffer_size_bytes
- self._remote_kw = remote_kw
+ self._session_name: str = session_name
Review Comment:
can you update the docstring to describe the range of acceptable values and the purpose?
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -86,7 +86,12 @@ def __enter__(self):
self._rpc_receive_buffer_size_bytes,
],
)
- self.device = self._rpc.hexagon(0)
+ if self._session_name == "cpu-rpc":
+ self.device = self._rpc.cpu(0)
+ elif self._session_name == "hexagon-rpc":
+ self.device = self._rpc.hexagon(0)
+ else:
+ raise RuntimeError(f"Incorrect session name: {self._session_name}")
Review Comment:
want to say something more about the options?
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -295,11 +305,19 @@ def _aot_executor_from_factory(
binary_name = "test_binary.so"
binary_path = temp_dir / binary_name
- module.export_library(
- str(binary_path),
- fcompile=hexagon.create_aot_shared,
- hexagon_arch=hexagon_arch,
- )
+ if target_kind == "hexagon":
+ module.export_library(
+ str(binary_path),
+ fcompile=hexagon.create_aot_shared,
+ hexagon_arch=hexagon_arch,
+ )
+ elif target_kind == "llvm":
+ module.export_library(
+ str(binary_path),
+ cc=hexagon.hexagon_clang_plus(),
+ )
+ else:
+ raise ValueError("Incorrect Target kind.")
Review Comment:
same--want to list the valid options?
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.cc:
##########
@@ -107,12 +107,16 @@ struct HexagonWorkspacePool : public WorkspacePool {
};
void* HexagonDeviceAPIv2::AllocWorkspace(Device dev, size_t size, DLDataType type_hint) {
- CHECK(TVMDeviceExtType(dev.device_type) == kDLHexagon) << "dev.device_type: " << dev.device_type;
+ bool is_valid_device = (TVMDeviceExtType(dev.device_type) == kDLHexagon) ||
Review Comment:
could you add a note explaining why kDLCPU is here?
##########
src/runtime/hexagon/rpc/hexagon/rpc_server.cc:
##########
@@ -58,11 +58,10 @@ class HexagonIOHandler {
read_buffer_size_bytes_{read_buffer_size_bytes},
write_buffer_available_length_{0} {}
- void MessageStart(size_t message_size_bytes) {}
+ void MessageStart(size_t message_size_bytes) { LOG(INFO) << "MessageStart called."; }
Review Comment:
revert or move to VLOG
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -304,11 +315,13 @@ def test_aot_executor(hexagon_session):
lowered = tvm.relay.build(
relay_mod,
params=params,
- target=tvm.target.Target(target_hexagon, host="c"),
+ target=tvm.target.Target(target_hexagon, host=aot_target_kind),
runtime=Runtime("cpp"),
executor=Executor("aot", {"unpacked-api": False, "interface-api": "packed"}),
)
+ hexagon_session = hexagon_launcher.start_session(name=session_key)
+ hexagon_session.__enter__()
Review Comment:
why call this instead of with hexagon_session:?
##########
src/runtime/hexagon/rpc/hexagon/rpc_server.cc:
##########
@@ -156,13 +155,19 @@ class HexagonRPCServer {
* \param data The data pointer
* \param data_size_bytes The data size in bytes.
*
- * \return The size of data written to IOHandler.
+ * \return The size of data written to IOHandler if no error.
+ * Otherwise, returns -1;
*/
int64_t Write(const uint8_t* data, size_t data_size_bytes) {
if (io_.SetReadBuffer(data, data_size_bytes) != AEE_SUCCESS) {
+ LOG(ERROR) << "ERROR: SetReadBuffer failed";
Review Comment:
can you include the error code?
##########
src/relay/backend/aot_executor_codegen.cc:
##########
@@ -1234,12 +1234,10 @@ class AOTExecutorCodegenModule : public runtime::ModuleNode {
Target target_host;
for (const auto& it : tmp) {
auto dev_type = it.first.as<tir::IntImmNode>();
- if (!target_host.defined() && it.second->kind->device_type == kDLCPU) {
+ if (!target_host.defined() && ((it.second->kind->device_type == kDLCPU) ||
+ (it.second->kind->device_type == kDLHexagon))) {
Review Comment:
could you add a note explaining why kDLHexagon is here?
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -270,8 +271,18 @@ def _workaround_create_aot_shared():
)
+def get_target_and_session(target_kind: str):
+ if target_kind == "c":
+ target_hexagon = tvm.target.hexagon("v68")
+ session_key = "hexagon-rpc"
+ elif target_kind.startswith("llvm"):
+ target_hexagon = target_kind
+ session_key = "cpu-rpc"
Review Comment:
add else: assert False, "explanation"
--
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