[Hexagon] AoT with LLVM Codegen on Hexagon by mehrdadh · Pull Request #11065 · apache/tvm

mehrdadh · 2022-04-19T19:47:44Z

This PR adds AoT executor implementation with LLVM codegen for Hexagon.

areusch

thanks @mehrdadh , a couple of comments

csullivan · 2022-04-20T16:55:09Z

+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


Good candidate for a test fixture

added. thanks!

csullivan · 2022-04-20T16:59:15Z


 @requires_hexagon_toolchain
-def test_aot_executor_multiple_conv2d(hexagon_session):
+def test_aot_executor_multiple_conv2d(hexagon_launcher, aot_target_kind):


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.

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.

Lunderberg

It looks like we have some complications due to information flowing backwards, from the device type to the session initialization. I think we can have a cleaner interface by delaying the device initialization until after we know which device type we have, and have some proposed changes that should bring it about.

Lunderberg · 2022-04-20T21:14:00Z


    session_name : str
-        Hexagon RPC session name.
+        Hexagon RPC session name. Options are [hexagon-rpc, cpu-rpc].


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.

Lunderberg

Thank you for making the changes, and it looks much better! I have a couple additional changes to request, after which it would look good to me.

Lunderberg · 2022-04-21T18:15:07Z

+    def device(self):
+        """Session device."""
+
+        if hasattr(self, "_device") and self._device is not None:


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.

Lunderberg · 2022-04-21T18:25:59Z

+        if hasattr(self, "_device") and self._device is not None:
+            return self._device
+
+        if not hasattr(self, "_requires_cpu_device"):


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).

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.

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

makes sense, added the default and changed set_device_type to be an internal function.

Lunderberg · 2022-04-21T18:28:59Z


        raise TypeError(f"Unsupported executor type: {type(module)}")

+    def set_device_type(self, module: Union[str, pathlib.Path, GraphExecutorFactoryModule]):


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.

Lunderberg · 2022-04-21T18:34:27Z

+        if not hasattr(self, "_requires_cpu_device"):
+            assert (
+                False
+            ), "Device type is not set. 'set_device_type' should be called before accessing device."


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.

changed set_device_type to be an internal function and removed the assert since it has default value now.

Lunderberg

Thank you for making the updates. LGTM!

Lunderberg · 2022-04-21T20:49:54Z

Looks like the linter wasn't recognizing the hasattr as ensuring that there was a definition for self._device. From this thread, 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.

mehrdadh · 2022-04-21T20:52:41Z

@areusch, @driazati is there a plan on updating pylint?

* AOT with LLVM Codegen on Hexagon * Address comments

github-actions Bot requested review from areusch, csullivan and kparzysz-quic April 19, 2022 19:48

mehrdadh force-pushed the hexagon/aot-llvm-final branch from 6ebbb79 to 40765de Compare April 19, 2022 19:48

mehrdadh changed the title ~~[Hexagon] AoT with LLVM Codegen for on Hexagon~~ [Hexagon] AoT with LLVM Codegen on Hexagon Apr 19, 2022

AOT with LLVM Codegen on Hexagon

3cd73c4

mehrdadh force-pushed the hexagon/aot-llvm-final branch from 40765de to 3cd73c4 Compare April 19, 2022 21:42

areusch reviewed Apr 19, 2022

View reviewed changes

masahi reviewed Apr 19, 2022

View reviewed changes

Comment thread src/runtime/hexagon/rpc/hexagon/rpc_server.cc

mehrdadh requested review from areusch and masahi April 20, 2022 15:59

Address comments

278a78c

mehrdadh force-pushed the hexagon/aot-llvm-final branch from b5c22b0 to 278a78c Compare April 20, 2022 16:03

csullivan reviewed Apr 20, 2022

View reviewed changes

address Chris comments

2ce8445

csullivan reviewed Apr 20, 2022

View reviewed changes

Comment thread tests/python/contrib/test_hexagon/conftest.py Outdated

Lunderberg requested changes Apr 20, 2022

View reviewed changes

Address Eric suggesions

31a7e80

mehrdadh force-pushed the hexagon/aot-llvm-final branch from 8c26cad to 31a7e80 Compare April 21, 2022 16:48

mehrdadh requested review from Lunderberg and csullivan April 21, 2022 16:52

Lunderberg requested changes Apr 21, 2022

View reviewed changes

address more comments

4c77cc5

mehrdadh requested a review from Lunderberg April 21, 2022 20:32

Lunderberg approved these changes Apr 21, 2022

View reviewed changes

fix lint

3d7d8c7

mehrdadh merged commit effc23d into apache:main Apr 21, 2022

mehrdadh deleted the hexagon/aot-llvm-final branch April 21, 2022 22:49

mehrdadh mentioned this pull request Apr 21, 2022

[Hexagon] Delete offload runtime, move files to right places #11090

Merged

shtinsa pushed a commit to Deelvin/tvm that referenced this pull request May 17, 2022

[Hexagon] AoT with LLVM Codegen on Hexagon (apache#11065)

44923c9

* AOT with LLVM Codegen on Hexagon * Address comments

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

cconvey mentioned this pull request Nov 4, 2022

[build][hexagon] remove unused variable #13291

Merged


		raise TypeError(f"Unsupported executor type: {type(module)}")

		def set_device_type(self, module: Union[str, pathlib.Path, GraphExecutorFactoryModule]):

Conversation

mehrdadh commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lunderberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Lunderberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lunderberg left a comment

Choose a reason for hiding this comment

Uh oh!

Lunderberg commented Apr 21, 2022

Uh oh!

mehrdadh commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mehrdadh commented Apr 19, 2022 •

edited

Loading