[Hexagon] Update hexagon API build instruction and cleanup hexagon_proxy_rpc #10068
Conversation
mehrdadh
requested review from
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
kazum,
liangfu,
masahi,
merrymercy,
tmoreau89,
tqchen,
vinx13,
yzhliu,
zhiics and
ZihengJiang
as code owners
|
@kparzysz-quic appreciate your review here. Also, I need to fix this hack before merge: https://github.com/apache/tvm/pull/10068/files#diff-1abd6a553915398cb46fbd7115d3427b8e87514da965925c8f31f141f86fc3a0R501 |
|
What happens without this hack? Are you running into errors building the Android runtime? Is it because of a missing FastRPC header? I see that on this line you set the Hexagon device to |
|
@kparzysz-quic thanks for the suggestion! Fixed it. |
|
cc @Lunderberg |
| @@ -26,20 +26,38 @@ Here are the steps that are taken to prepare a runtime on a Hexagon device to te | |||
| - Build TVM library with Hexagon support for host machine. | |||
| - Build TVMRuntime library and C++ RPC server for host machine. | |||
|
|
|||
| To build these pieces, you can use a cmake command as follow. | |||
| Note: before moving forward make sure to export Clang libraries to `LD_LIBRARY_PATH` and Hexagon toolchain to `HEXAGON_TOOLCHAIN`. | |||
There was a problem hiding this comment.
A few suggestions:
-
nitpick: "before moving forward" is an unusual phrasing in a file like this, and parsing it slowed down my reading a little bit. It might be more idiomatic to say something like, "First, ensure that ...".
-
For variables like these, sometimes people get confused regarding how deep into a directory structure the variables should point. It might help to give concrete examples to clarify that.
There was a problem hiding this comment.
added more details. thanks!
| cd apps/hexagon_api | ||
| mkdir build | ||
| cd build | ||
| cmake -DUSE_ANDROID_TOOLCHAIN=/path/to/android-ndk/build/cmake/android.toolchain.cmake \ |
There was a problem hiding this comment.
Just a personal opinion, but I'd be careful with this style. It's formatted as bash code, and the first few lines look like literal bash code. So some people (probably me included) will potentially copy this whole block and paste it into a terminal, without reading it carefully. That's a problem because the following lines aren't literal bash commands, but rather human-readable bash pseudocode.
There was a problem hiding this comment.
I agree with you. Changed the style, PTAL.
| cd tvm | ||
| mkdir build | ||
| cd build | ||
| cmake -DUSE_LLVM=/path/to/llvm/bin/llvm-config \ |
There was a problem hiding this comment.
(Same comment as above, regarding mixing literal bash code with bash pseudo-code in a bash-formatted code block.
|
@cconvey thanks for the review. I addressed your comments, PTAL. |
…oxy_rpc (apache#10068) * Fix hexagon api build and Update Readme * Cleanup hexagon_proxy_rpc * Target Hack * Remove hack * address @cconvey comments * remove the rest of proxy rpc
…oxy_rpc (apache#10068) * Fix hexagon api build and Update Readme * Cleanup hexagon_proxy_rpc * Target Hack * Remove hack * address @cconvey comments * remove the rest of proxy rpc
This PR: