[Hexagon] Add RPC Mechanism for Hexagon by mehrdadh · Pull Request #9631 · apache/tvm

mehrdadh · 2021-12-01T19:46:43Z

This PR:

Reuses minRPC implementation for hexagon to create RPC connection from host to hexagon device which is passed through android RPC server.
Adds HexagonLauncher which manages file upload to Android, initiate RPC server on Android and manage RPC session to hexagon.

To use this feature you need to build TVM using these configs:

cmake -DUSE_HEXAGON_RPC=ON \
       -DUSE_ANDROID_TOOLCHAIN=/path/to/android-ndk/build/cmake/android.toolchain.cmake \
       -DANDROID_PLATFORM=android-28 \
       -DANDROID_ABI=arm64-v8a \
       -DUSE_HEXAGON_ARCH=v65|v66|v68 \
       -DUSE_HEXAGON_SDK=/path/to/Hexagon/SDK \
       -DUSE_HEXAGON_TOOLCHAIN=/path/to/Hexagon/toolchain/ \
       -DUSE_LLVM=/path/to/llvm/bin/llvm-config \
       -DUSE_CPP_RPC=ON \
       -DCMAKE_CXX_COMPILER=/path/to/clang++ \    
       -DCMAKE_CXX_FLAGS='-stdlib=libc++' ..

This will build:

libtvm and libtvm_runtime using hexagon LLVM andtvm_rcp for host.
libtvm_runtime_android.so and tvm_rpc_android for Android under build/hexagon_rpc
libhexagon_rpc_skel.so library which include RPC server for Hexagon under build/hexagon_rpc

cc @areusch @csullivan @adstraw

csullivan

What a wonderful consolidation of effort for bringing all the TVM runtimes to Hexagon. The simplicity of the FastRPC interface and resulting enabled featureset speak for themselves. Truly incredible work @mehrdadh, thank you!

csullivan · 2021-12-03T22:20:39Z

+  # copy android_bash template file
+  configure_file("${CMAKE_SOURCE_DIR}/src/runtime/hexagon/rpc/android_bash.sh.template" 
+    ${HEXAGON_RPC_OUTPUT} COPYONLY)
+endif()


Clean up artifacts, e.g.

Suggested change

endif()

set_directory_properties(PROPERTIES ADDITIONAL_MAKE_CLEAN_FILES "${HEXAGON_RPC_OUTPUT}")

endif()

done here: 28db608

csullivan · 2021-12-03T22:36:07Z

    INSTALL_COMMAND "true"
  )
-elseif(USE_HEXAGON_DEVICE STREQUAL "${PICK_HW}")
+elseif((USE_HEXAGON_DEVICE STREQUAL "${PICK_HW}") OR (USE_HEXAGON_RPC AND BUILD_FOR_ANDROID))


Can we decouple the (USE_HEXAGON_DEVICE STREQUAL "${PICK_HW}") and (USE_HEXAGON_RPC AND BUILD_FOR_ANDROID) cases into separate conditionals? It's a bit tricky to follow the flow and easy to make an error in the build for one or the other flows unintentionally when grouping them together like this.

done: 28db608

csullivan · 2021-12-03T22:37:55Z

  elseif(BUILD_FOR_ANDROID AND HEXAGON_SDK_PATH_DEFINED)
    list(APPEND RUNTIME_HEXAGON_SRCS src/runtime/hexagon/proxy_rpc/device_api.cc)
-  else()
+  elseif(USE_HEXAGON_RPC)


See comment ⬆️ about breaking out not reusing the USE_HEXAGON_DEVICE block. It will also be easier to delete the USE_HEXAGON_DEVICE flow down the line if they are decoupled.

done: 28db608

csullivan · 2021-12-04T00:19:59Z

+         read_buffer_size_bytes_);
+
+    uint32_t bytes_to_read = 0;
+    if ((read_buffer_size_bytes_ - read_len_bytes) < 0) {


read_buffer_size_bytes_ can be uninitialized if SetReadBuffer is not called.

I initialized it at the constructor as you suggested in previous comment.

csullivan · 2021-12-04T00:22:30Z

+static tvm::runtime::hexagon::HexagonRPCServer* g_hexagon_rpc_server = nullptr;
+
+static AEEResult hexagon_rpc_server_init() {
+  uint8_t* receive_buffer = new uint8_t[TVM_HEXAGON_RPC_BUFF_SIZE_BYTES];


Nothing as far as I can tell is protecting against overflowing the receive_buffer TVM_HEXAGON_RPC_BUFF_SIZE_BYTES, we don't bass the receive_buffer_size to the iohandler (via the server).

I added check for buffer size and passed the size to IOHandler:
31fda2b

csullivan · 2021-12-04T00:26:52Z

+}  // namespace runtime
+}  // namespace tvm
+
+static tvm::runtime::hexagon::HexagonRPCServer* g_hexagon_rpc_server = nullptr;


Use an anonymous function to return the static global and init on first use

can you clarify this?

addressed here: 31fda2b

csullivan · 2021-12-04T00:34:02Z

+@tvm.testing.fixture
+def tvm_tracker_host():
+    return os.environ["TVM_TRACKER_HOST"]
+
+
+@tvm.testing.fixture
+def tvm_tracker_port():
+    return int(os.environ["TVM_TRACKER_PORT"])
+
+
+def _compose(args, decs):
+    """Helper to apply multiple markers"""
+    if len(args) > 0:
+        f = args[0]
+        for d in reversed(decs):
+            f = d(f)
+        return f
+    return decs
+
+
+def requires_rpc_tracker(*args):
+    """Mark a test as requiring an RPC tracker to exist in
+    the host environment to run."""
+    _requires_rpc_tracker = [
+        *tvm.testing.requires_rpc(),
+        pytest.mark.skipif(
+            os.environ.get("TVM_TRACKER_HOST") == None,
+            reason="Missing environment variable, TVM_TRACKER_HOST",
+        ),
+        pytest.mark.skipif(
+            os.environ.get("TVM_TRACKER_PORT") == None,
+            reason="Missing environment variable, TVM_TRACKER_PORT",
+        ),
+    ]
+
+    return _compose(args, _requires_rpc_tracker)
+
+
+def requires_hexagon_toolchain(*args):
+    _requires_hexagon_toolchain = [
+        pytest.mark.skipif(
+            os.environ.get("HEXAGON_TOOLCHAIN") == None,
+            reason="HEXAGON_TOOLCHAIN environment variable is required to run this test.",
+        ),
+    ]


These are defined also in test_hexagon/proxy_rpc/ for the sake of common code can you put these in test_hexagon/conftest.py?

Refactored conftest files here: aa61967

adstraw

This is a partial review to note that I tested out this PR using the README and was able to get it running. LGTM from a usability / documentation point of view.

csullivan

Looks great to me! Only (non-blocking) nitpicks/TODOs

* Add Hexagon RPC * removed android remote and updated Readme * Add check for workspace size * Make libtvm_runtime consistent for Android * Remove root access * Fix some docstrings * Make stack remote size as parameter * add documentation * Refactor test conftest * clang format * Decoupled USE_HEXAGON_RPC * fix creation of test base directory on android * Address global variable * Fix format and Cleanup cmake * Fix build for other targets

kparzysz-quic · 2021-12-13T21:04:29Z

+    COMMAND ${QAIC_EXE} ${QAIC_FLAGS} "${HEXAGON_RPC_DIR}/${RPC_IDL}" -o ${HEXAGON_RPC_DIR}
+    MAIN_DEPENDENCY "${HEXAGON_RPC_DIR}/${RPC_IDL}"
+  )
+  file(GLOB HEXAGON_RPC_CPP "${HEXAGON_RPC_DIR}/android/*.cc")


This will unconditionally add all files from that directory to the runtime target. If USE_HEXAGON_RPC is not set, it will cause link errors.

@kparzysz-quic thanks for pointing out. This could break if USE_HEXAGON_SDK and BUILD_FOR_ANDROID are enabled and USE_HEXAGON_RPC not enabled. So we should fix this. I suggest that we fix this with a cleanup of Hexagon.cmake once we agreed on using hexagon RPC and decided to deprecate hexagon proxy rpc and apps/hexagon_launcher.

@kparzysz-quic happy to hear your suggestion on this. thanks!

* Add Hexagon RPC * removed android remote and updated Readme * Add check for workspace size * Make libtvm_runtime consistent for Android * Remove root access * Fix some docstrings * Make stack remote size as parameter * add documentation * Refactor test conftest * clang format * Decoupled USE_HEXAGON_RPC * fix creation of test base directory on android * Address global variable * Fix format and Cleanup cmake * Fix build for other targets

kparzysz-quic · 2022-01-27T21:52:10Z

+  }
+
+  /*!
+   * \brief Get pointer to the buffer that a packet has been written to.


This function doesn't "get any pointers", it copies (part of) the write buffer to the specified address.

kparzysz-quic · 2022-01-27T21:53:45Z

+                  read_buffer_index_);
+
+    uint32_t bytes_to_read = 0;
+    if ((read_buffer_index_ - read_len_bytes) < 0) {


This shouldn't be subtracting unsigned numbers, just comparing them.

kparzysz-quic · 2022-01-27T21:57:16Z

+    read_buffer_index_ -= bytes_to_read;
+    if (bytes_to_read != read_len_bytes) {
+      HEXAGON_PRINT(ERROR, "Error bytes_to_read (%d) < read_len_bytes (%d).", bytes_to_read,
+                    read_len_bytes);


If it's an error to read less than read_len_bytes, then why is the code above trying to handle this?

kparzysz-quic · 2022-01-27T21:58:02Z

+    if (data_size_bytes > read_buffer_size_bytes_) {
+      return AEE_EFAILED;
+    }
+    read_buffer_ = data;


This loses the initial read buffer, which is never deallocated.

kparzysz-quic · 2022-01-27T21:58:49Z

+ */
+class HexagonIOHandler {
+ public:
+  explicit HexagonIOHandler(uint8_t* read_buffer, size_t read_buffer_size_bytes)


The read_buffer is not used for anything.

kparzysz-quic · 2022-01-27T21:59:32Z

+  explicit HexagonIOHandler(uint8_t* read_buffer, size_t read_buffer_size_bytes)
+      : read_buffer_{read_buffer},
+        read_buffer_size_bytes_{read_buffer_size_bytes},
+        read_buffer_index_{0} {}


The order here should follow the order of declarations below.

kparzysz-quic · 2022-01-27T22:00:50Z

+namespace {
+tvm::runtime::hexagon::HexagonRPCServer* get_hexagon_rpc_server() {
+  static tvm::runtime::hexagon::HexagonRPCServer g_hexagon_rpc_server(
+      new uint8_t[TVM_HEXAGON_RPC_BUFF_SIZE_BYTES], TVM_HEXAGON_RPC_BUFF_SIZE_BYTES);


This allocation is lost the first time the server tries to read something (see SetReadBuffer).

mehrdadh · 2022-01-28T21:35:57Z

@kparzysz-quic thanks for pointing out these issues. I will address these in the next PR for AOT executor.

* Add Hexagon RPC * removed android remote and updated Readme * Add check for workspace size * Make libtvm_runtime consistent for Android * Remove root access * Fix some docstrings * Make stack remote size as parameter * add documentation * Refactor test conftest * clang format * Decoupled USE_HEXAGON_RPC * fix creation of test base directory on android * Address global variable * Fix format and Cleanup cmake * Fix build for other targets

mehrdadh force-pushed the hexagon/hexagon_rpc branch 2 times, most recently from 5fc7750 to 129d96a Compare December 2, 2021 03:23

mehrdadh marked this pull request as ready for review December 2, 2021 03:25

mehrdadh requested review from a team, ZihengJiang, areusch, comaniac, icemelon, jroesch, junrushao, kazum, kparzysz-quic, liangfu, masahi, merrymercy, tmoreau89, tqchen, vinx13, yzhliu and zhiics as code owners December 2, 2021 03:25

mehrdadh force-pushed the hexagon/hexagon_rpc branch 3 times, most recently from 38c2c50 to ef7da6e Compare December 2, 2021 22:36

Add Hexagon RPC

6f2eec9

mehrdadh force-pushed the hexagon/hexagon_rpc branch from 2f0bd36 to 6f2eec9 Compare December 3, 2021 08:23

jroesch assigned areusch Dec 3, 2021

jroesch added the status: need review label Dec 3, 2021

removed android remote and updated Readme

87d8bec

csullivan requested changes Dec 4, 2021

View reviewed changes

Add check for workspace size

cea8757

fix creation of test base directory on android

f87f51d

adstraw approved these changes Dec 7, 2021

View reviewed changes

Address global variable

31fda2b

mehrdadh force-pushed the hexagon/hexagon_rpc branch from d4feb90 to 8216120 Compare December 7, 2021 23:34

Fix format and Cleanup cmake

a7ee1a9

mehrdadh force-pushed the hexagon/hexagon_rpc branch from 8216120 to a7ee1a9 Compare December 7, 2021 23:41

csullivan reviewed Dec 8, 2021

View reviewed changes

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

csullivan approved these changes Dec 8, 2021

View reviewed changes

Fix build for other targets

532bf1b

mehrdadh force-pushed the hexagon/hexagon_rpc branch from 269b2a8 to 532bf1b Compare December 8, 2021 01:53

jroesch approved these changes Dec 8, 2021

View reviewed changes

jroesch merged commit cd2fa69 into apache:main Dec 8, 2021

mehrdadh mentioned this pull request Dec 8, 2021

Follow up on PR #9631 #9685

Closed

mehrdadh mentioned this pull request Dec 10, 2021

[LLVM][Hexagon] Fix LLVM version #9711

Merged

kparzysz-quic reviewed Dec 13, 2021

View reviewed changes

kparzysz-quic reviewed Jan 27, 2022

View reviewed changes

mehrdadh deleted the hexagon/hexagon_rpc branch January 28, 2022 22:52

mehrdadh added a commit to mehrdadh/tvm that referenced this pull request Feb 9, 2022

Fixes for follow up on PR apache#9631

51484e6

mehrdadh added a commit to mehrdadh/tvm that referenced this pull request Feb 9, 2022

Fixes for follow up on PR apache#9631

000ab01

kparzysz-quic pushed a commit that referenced this pull request Feb 11, 2022

Fixes for follow up on PR #9631 (#10205)

c20cbc5

ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022

Fixes for follow up on PR apache#9631 (apache#10205)

cf33215

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022

Fixes for follow up on PR apache#9631 (apache#10205)

0eec5c5

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

	endif()
	set_directory_properties(PROPERTIES ADDITIONAL_MAKE_CLEAN_FILES "${HEXAGON_RPC_OUTPUT}")
	endif()

Conversation

mehrdadh commented Dec 1, 2021

Uh oh!

csullivan 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adstraw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csullivan 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!

mehrdadh commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants