[GitHub] [tvm] csullivan commented on a diff in pull request #11065: [Hexagon] AoT with LLVM Codegen on Hexagon

[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