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 19:47:45 UTC
[GitHub] [tvm] mehrdadh opened a new pull request, #11065: [Hexagon] AoT with LLVM Codegen for on Hexagon
mehrdadh opened a new pull request, #11065:
URL: https://github.com/apache/tvm/pull/11065
This PR adds AoT executor examples with LLVM codegen for Hexagon.
cc @areusch @kparzysz-quic @csullivan
--
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] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855394885
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -286,6 +296,11 @@ def _aot_executor_from_factory(
for target in module.target.values()
if "hexagon" in target.keys
)
Review Comment:
this is a much better approach, thanks for the suggestion. I've added it. PTAL
--
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] mehrdadh commented on pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11065:
URL: https://github.com/apache/tvm/pull/11065#issuecomment-1105748123
@areusch, @driazati is there a plan on updating pylint?
--
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] areusch commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [tvm] csullivan commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r854344087
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -286,6 +294,11 @@ def _aot_executor_from_factory(
for target in module.target.values()
Review Comment:
nit: Need some clean up [here](https://github.com/apache/tvm/pull/11065/files#diff-af4d2019fb7bd2f88bd9209309d4008514cdf340047d3019b41e8f4fbcbbdac0R281) and [here](https://github.com/apache/tvm/pull/11065/files#diff-af4d2019fb7bd2f88bd9209309d4008514cdf340047d3019b41e8f4fbcbbdac0R287), to reference AOT not graph.
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -332,7 +346,7 @@ def test_aot_executor(hexagon_session):
@requires_hexagon_toolchain
-def test_aot_executor_multiple_conv2d(hexagon_session):
+def test_aot_executor_multiple_conv2d(hexagon_launcher, aot_target_kind):
Review Comment:
Can we instead support aot_target_kind and its consumption via `get_target_and_session` in the hexagon_session fixture? That way we can keep the simplified calling code so that the user doesn't need to think about starting a hexagon session.
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -270,8 +271,20 @@ def _workaround_create_aot_shared():
)
+def get_target_and_session(target_kind: str):
+ if target_kind == "c":
+ target_hexagon = tvm.target.hexagon("v68")
+ session_name = "hexagon-rpc"
+ elif target_kind.startswith("llvm"):
+ target_hexagon = target_kind
+ session_name = "cpu-rpc"
+ else:
+ assert False, "Incorrect target_kind: {target_kind}. Options are [c, llvm]."
+ return target_hexagon, session_name
Review Comment:
Good candidate for a test fixture
##########
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:
I think it's probably also helpful to have a comment in the docstring about why there are two variants. e.g. running with hexagon as an llvm subtarget
--
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] Lunderberg commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855492678
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
Also, I think this default is required for some use cases. The `load_module()` function doesn't call `set_device_type`, so I expect the TE-based tests to fail. It is also legal to upload a module, and later refer to the uploaded module by its name. Since this can occur on an entirely different instance of TVM, there wouldn't be a way to determine the device type, and we can't avoid needing a default.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
Review Comment:
I'd recommend either removing the `hasattr(self, "_device")` from this condition, or removing `self._device = None` from the initializer. As it is, there are two different ways to represent an uninitialized session, which could cause confusion in the future.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -226,6 +244,28 @@ def get_executor_from_factory(self, module: ExecutorFactoryModule):
raise TypeError(f"Unsupported executor type: {type(module)}")
+ def set_device_type(self, module: Union[str, pathlib.Path, GraphExecutorFactoryModule]):
Review Comment:
Should `set_device_type` be a private function? If it is only needed from the AOT path, and should only be called from within `_get_aot_executor_from_factory`, we should rename to `_set_device_type`.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
+ assert (
+ False
+ ), "Device type is not set. 'set_device_type' should be called before accessing device."
Review Comment:
This error message instructs users to call `set_device_type`, which we may want to be an internal function. The concern would be for future breakage, if code outside this class calls `hexagon_session.set_device_type`, we wouldn't be able to remove `set_device_type` without breaking that code. I think there are some plans to remove the `kDLHexagon` type and instead use options within the `kDLCPU`, at which point we'd want to remove the device choice without breaking external usage.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
Do we want to supply a default for `_requires_cpu_device`, rather than requiring it to be explicitly set? I think general case is that we return a "hexagon" device, and that AOT is the special case that would require a CPU device. Rather than requiring it to be set for both paths, can we set `self._requires_cpu_device = False` in the initializer, and only set it to True in the AOT path. That way, the default is to return a "hexagon" device.
Also, as a general nitpick, I'd recommend `assert(condition)` instead of `if not condition: assert(False)`.
--
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] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855542579
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
Review Comment:
done.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -226,6 +244,28 @@ def get_executor_from_factory(self, module: ExecutorFactoryModule):
raise TypeError(f"Unsupported executor type: {type(module)}")
+ def set_device_type(self, module: Union[str, pathlib.Path, GraphExecutorFactoryModule]):
Review Comment:
done.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
+ assert (
+ False
+ ), "Device type is not set. 'set_device_type' should be called before accessing device."
Review Comment:
changed set_device_type to be an internal function and removed the assert since it has default value now.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
makes sense, added the default and changed set_device_type to be an internal 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855392563
##########
tests/python/contrib/test_hexagon/conftest.py:
##########
@@ -202,3 +202,29 @@ def terminate_rpc_servers():
yield []
if serial == "simulator":
os.system("ps ax | grep tvm_rpc_x86 | awk '{print $1}' | xargs kill")
+
+
+aot_host_target = tvm.testing.parameter(
+ "c",
+ "llvm -keys=hexagon -link-params=0 -mattr=+hvxv68,+hvx-length128b,+hvx-qfloat,-hvx-ieee-fp -mcpu=hexagonv68 -mtriple=hexagon",
+)
+
+
+@pytest.fixture()
Review Comment:
done, thanks!
--
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] Lunderberg commented on pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11065:
URL: https://github.com/apache/tvm/pull/11065#issuecomment-1105745641
Looks like the linter wasn't recognizing the `hasattr` as ensuring that there was a definition for `self._device`. From [this thread](https://github.com/PyCQA/pylint/issues/2315#issuecomment-628331369), it looks like that's a bug with pylint, fixed at some point between March 2020 and December 2021. The CI is currently running pylint 2.4.4, released in November 2019, so it wouldn't be unreasonable to update the pylint version at some point.
Updating to use `self._device = None` instead of `hasattr` LGTM.
--
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] Lunderberg commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r854559605
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -44,7 +44,9 @@ class Session:
Remote configs for RPC tracker.
session_name : str
- Hexagon RPC session name.
+ Hexagon RPC session name. Options are [hexagon-rpc, cpu-rpc].
Review Comment:
I'm somewhat concerned with the change in semantics here. Going from a human-readable name that can be arbitrarily specified to a list of specific options feels like there's got to be a better way.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -86,7 +88,15 @@ def __enter__(self):
self._rpc_receive_buffer_size_bytes,
],
)
- self.device = self._rpc.hexagon(0)
+ if self._session_name == "cpu-rpc":
Review Comment:
Proposed option: Change `device` to be lazily generated in a `@property`, rather than set during the `__enter__` method.
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -286,6 +296,11 @@ def _aot_executor_from_factory(
for target in module.target.values()
if "hexagon" in target.keys
)
Review Comment:
Around here, similar to how the hexagon architecture gets pulled out, we should also pull out the target host. If it is "llvm", we can set `self._requires_cpu_device = True` (or some better-named variable). Then it can be referred to in the device property
```python
@property
def device(self):
if hasattr(self, '_device'):
return self._device
if self._requires_cpu_device:
self._device = self._rpc.cpu(0)
else:
self._device = self._rpc.hexagon(0)
return self._device
```
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -86,7 +88,15 @@ def __enter__(self):
self._rpc_receive_buffer_size_bytes,
],
)
- self.device = self._rpc.hexagon(0)
+ if self._session_name == "cpu-rpc":
Review Comment:
This will require some of the `if self._device is None` checks to be replaced with `if self._rpc is None`.
--
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] masahi commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r853610045
##########
src/runtime/hexagon/rpc/hexagon/rpc_server.cc:
##########
@@ -211,6 +216,7 @@ const tvm::runtime::PackedFunc get_runtime_func(const std::string& name) {
void reset_device_api() {
const tvm::runtime::PackedFunc api = get_runtime_func("device_api.hexagon.v2");
tvm::runtime::Registry::Register("device_api.hexagon", true).set_body(api);
+ tvm::runtime::Registry::Register("device_api.cpu", true).set_body(api);
Review Comment:
Just curious, why this is necessary? A comment would be nice.
--
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] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r854277144
##########
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:
done.
##########
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:
done.
##########
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:
done.
##########
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:
done.
##########
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:
done.
##########
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:
done.
##########
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:
done.
##########
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:
done
##########
src/runtime/hexagon/rpc/hexagon/rpc_server.cc:
##########
@@ -211,6 +216,7 @@ const tvm::runtime::PackedFunc get_runtime_func(const std::string& name) {
void reset_device_api() {
const tvm::runtime::PackedFunc api = get_runtime_func("device_api.hexagon.v2");
tvm::runtime::Registry::Register("device_api.hexagon", true).set_body(api);
+ tvm::runtime::Registry::Register("device_api.cpu", true).set_body(api);
Review Comment:
done.
##########
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:
done.
##########
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:
changed.
--
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] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855392885
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -86,7 +88,15 @@ def __enter__(self):
self._rpc_receive_buffer_size_bytes,
],
)
- self.device = self._rpc.hexagon(0)
+ if self._session_name == "cpu-rpc":
Review Comment:
done.
--
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] mehrdadh merged pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #11065:
URL: https://github.com/apache/tvm/pull/11065
--
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] Lunderberg commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r855496310
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -95,6 +94,25 @@ def __enter__(self):
def __exit__(self, exc_type, exc_value, exc_traceback):
pass
+ @property
+ def device(self):
+ """Session device."""
+
+ if hasattr(self, "_device") and self._device is not None:
+ return self._device
+
+ if not hasattr(self, "_requires_cpu_device"):
Review Comment:
Current CI results have some failures for `test_2d_physical_buffers` and `test_cache_read_write`, which look to be caused by this lack of a default. https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-11065/8/tests
--
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] mehrdadh commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r854410756
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -332,7 +346,7 @@ def test_aot_executor(hexagon_session):
@requires_hexagon_toolchain
-def test_aot_executor_multiple_conv2d(hexagon_session):
+def test_aot_executor_multiple_conv2d(hexagon_launcher, aot_target_kind):
Review Comment:
so AoT host targets are pytest parameters and they are not used for graph executor. If I add it in `hexagon_session` it will generate extra tests for graph executor which doesn't make sense. I added aot_target and aot_host_target as fixtures.
We could fix this by adding aot_hexagon_session fixture.
##########
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:
added.
##########
tests/python/contrib/test_hexagon/test_launcher.py:
##########
@@ -270,8 +271,20 @@ def _workaround_create_aot_shared():
)
+def get_target_and_session(target_kind: str):
+ if target_kind == "c":
+ target_hexagon = tvm.target.hexagon("v68")
+ session_name = "hexagon-rpc"
+ elif target_kind.startswith("llvm"):
+ target_hexagon = target_kind
+ session_name = "cpu-rpc"
+ else:
+ assert False, "Incorrect target_kind: {target_kind}. Options are [c, llvm]."
+ return target_hexagon, session_name
Review Comment:
added. thanks!
##########
python/tvm/contrib/hexagon/session.py:
##########
@@ -286,6 +294,11 @@ def _aot_executor_from_factory(
for target in module.target.values()
Review Comment:
Can you clarify where these two links point at?
--
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 #11065: [Hexagon] AoT with LLVM Codegen on Hexagon
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #11065:
URL: https://github.com/apache/tvm/pull/11065#discussion_r854409022
##########
tests/python/contrib/test_hexagon/conftest.py:
##########
@@ -202,3 +202,29 @@ def terminate_rpc_servers():
yield []
if serial == "simulator":
os.system("ps ax | grep tvm_rpc_x86 | awk '{print $1}' | xargs kill")
+
+
+aot_host_target = tvm.testing.parameter(
+ "c",
+ "llvm -keys=hexagon -link-params=0 -mattr=+hvxv68,+hvx-length128b,+hvx-qfloat,-hvx-ieee-fp -mcpu=hexagonv68 -mtriple=hexagon",
+)
+
+
+@pytest.fixture()
Review Comment:
```suggestion
@tvm.testing.fixture
```
and similarly else where
--
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