Skip to content

[Hexagon] Don't build RPC fastrpc libraries when USE_HEXAGON_RPC=OFF#9904

Closed
kparzysz-quic wants to merge 1 commit into
apache:mainfrom
kparzysz-quic:use-hexagon-rpc
Closed

[Hexagon] Don't build RPC fastrpc libraries when USE_HEXAGON_RPC=OFF#9904
kparzysz-quic wants to merge 1 commit into
apache:mainfrom
kparzysz-quic:use-hexagon-rpc

Conversation

@kparzysz-quic

Copy link
Copy Markdown
Contributor

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

@mehrdadh mehrdadh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kparzysz-quic this change would break the current hexagon launcher tests. USE_HEXAGON_RPC is used to initiate two external project builds in cmake/modules/Hexagon.cmake for hexagon and android sides. When we build the android side as an external project, this flag is not enabled. Here is a traceback of the error:

E       RuntimeError: Cannot request hexagon-dev after 5 retry, last_error:Traceback (most recent call last):
E         8: TVMFuncCall
E         7: std::__1::__function::__func<tvm::runtime::$_0, std::__1::allocator<tvm::runtime::$_0>, void (tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*)>::operator()(tvm::runtime::TVMArgs&&, tvm::runtime::TVMRetValue*&&)
E         6: tvm::runtime::RPCClientConnect(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, tvm::runtime::TVMArgs)
E         5: tvm::runtime::RPCConnect(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, tvm::runtime::TVMArgs)
E         4: tvm::runtime::RPCEndpoint::InitRemoteSession(tvm::runtime::TVMArgs)
E         3: tvm::runtime::RPCEndpoint::HandleUntilReturnEvent(bool, std::__1::function<void (tvm::runtime::TVMArgs)>)
E         2: tvm::runtime::RPCEndpoint::EventHandler::HandleNextEvent(bool, bool, std::__1::function<void (tvm::runtime::TVMArgs)>)
E         1: tvm::runtime::RPCEndpoint::EventHandler::HandleProcessPacket(std::__1::function<void (tvm::runtime::TVMArgs)>)
E         0: tvm::runtime::RPCEndpoint::EventHandler::HandleReturn(tvm::runtime::RPCCode, std::__1::function<void (tvm::runtime::TVMArgs)>)
E         File "/home/mhessar/work/tvm/src/runtime/rpc/rpc_endpoint.cc", line 376
E       RPCError: Error caught from RPC call:
E       [15:04:46] /home/mhessar/work/tvm/src/runtime/rpc/rpc_endpoint.cc:523: 
E       ---------------------------------------------------------------
E       An error occurred during the execution of TVM.
E       For more information, please see: https://tvm.apache.org/docs/errors.html
E       ---------------------------------------------------------------
E         Check failed: (fconstructor != nullptr) is false:  Cannot find session constructor tvm.contrib.hexagon.create_hexagon_session

@kparzysz-quic

kparzysz-quic commented Jan 13, 2022

Copy link
Copy Markdown
Contributor Author

As it is now, it breaks TVM builds that do not set that variable, because the guarded parts want to use functions that were not compiled.

I don't think it's a good idea to have the TVM build automatically build any apps. It happens to work with the launcher, but it's probably a bad precedent. If anything, the reverse should be the primary scenario---apps should build TVM runtimes configured to the apps' needs.

I'm going to remove the external projects from Hexagon.cmake. That should simplify the .cmake file at the expense of a bit of inconvenience.

When we auto build an app, the crux of the issue is whether we want to use the currently built TVM runtime with that app. If so, then we need to make sure that the specified TVM build configuration meets the app's expectations, which is something that TVM's cmake files should not be bothered doing.

Edit: added more rationale for not auto-building apps.

@kparzysz-quic

Copy link
Copy Markdown
Contributor Author

This PR will not be committed as is, but I'm keeping it open until the replacement PR is created. The reason is that it doesn't disappear from the list of PRs, in case someone wants to comment on this issue.

@mehrdadh

Copy link
Copy Markdown
Member

I think we need to simplify the Hexagon.cmake and that will happen anyway when we cleanup/remove apps/hexagon_proxy_rpc and apps/hexagon_launcher. I recommend we delay these major cleanups until we have enough testing in CI to avoid breaking build and also internal CI for hardware testing.

@kparzysz-quic

Copy link
Copy Markdown
Contributor Author

The problem for us is that it conflicts with our downstream code. It would actually be easier to do the cleanup first, since once CI is in place, we'd have to fix more things. Are you currently running some automatic tests that use this code?

@kparzysz-quic kparzysz-quic deleted the use-hexagon-rpc branch January 21, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants