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

[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