Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

kparzysz-quic
Copy link
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.

Copy link
Member

@mehrdadh mehrdadh left a comment

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
Copy link
Contributor Author

kparzysz-quic commented Jan 13, 2022

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