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

allow libbacktrace to be used when cross compiling the runtime #7917

Merged
merged 20 commits into from
Jun 1, 2021

Conversation

mherkazandjian
Copy link
Contributor

This PR fixes issue #7916

to build for aarch64 the cmake variable -DCMAKE_HOST=aarch64-linux-gnu should be passed that
is in turn propagated to the autotools (configure step) for libbacktrace.

cmake .. \
    -DCMAKE_SYSTEM_NAME=Linux \
    -DCMAKE_SYSTEM_VERSION=1 \
    -DCMAKE_C_COMPILER=/usr/bin/aarch64-linux-gnu-gcc-8 \
    -DCMAKE_CXX_COMPILER=/usr/bin/aarch64-linux-gnu-g++-8 \
    -DCMAKE_FIND_ROOT_PATH=/usr/aarch64-linux-gnu \
    -DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER \
    -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY \
    -DCMAKE_HOST=aarch64-linux-gnu  

I tested this to build the runtime for aarch64 and riscv64, the runtime .so was produced in both
cases with no errors displayed during the build.

@tqchen
Copy link
Member

tqchen commented Apr 24, 2021

@areusch @tkonolige @leandron please help to review this PR

@junrushao
Copy link
Member

CC @tkonolige @xiebaiyuan The issue is possibly related to #7705.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. As the default value for libbacktrace is AUTO, this patch enables cross-compilation of the source in the original cloning state - great!

On a broader view, it would be awesome to also update the documentation such as https://tvm.apache.org/docs/install/from_source.html, with some guidance for cross-compilation as there are non-obvious steps to be taken, such as the need for -DCMAKE_HOST=... to be populated, for example.

What do you think?

cmake/libs/Libbacktrace.cmake Outdated Show resolved Hide resolved
@leandron
Copy link
Contributor

On another note, i think an end to end example on inference on a remote machine would be great.
i could probably contribute that end to end example if it does not exist, does it?

Yes, that would be really useful, specially if it uses TVMC rather than low level APIs, IMHO.

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@tkonolige
Copy link
Contributor

btw, I tried these same changes when debugging #7705 but they did not fix it.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @mherkazandjian for the PR! just one question, I agree with @leandron that it would be great to provide some docs.

cmake/libs/Libbacktrace.cmake Show resolved Hide resolved
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

A few suggestions on the tutorial wording, feel free to decide whether you want to use it. @hogepodge recently did some work on tutorials and might be interested as well.

Apart from the suggestions, LGTM.

docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
docs/install/from_source.rst Outdated Show resolved Hide resolved
@mherkazandjian
Copy link
Contributor Author

@leandron tnx for the feedback. actually the CI build is failing, i think the in the CI we (I) should modify the cmake flags so that
libbacktrace now builds (it is failing in the CI). I will do it now.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

I happy to approve this PR. Did you check @tkonolige's comment about building with debug flags?

@mherkazandjian
Copy link
Contributor Author

I happy to approve this PR. Did you check @tkonolige's comment about building with debug flags?

great. thank you.
i looked into the comment of @tkonolige but i thought it is not strictly related to this issue in the sense that
this PR would not affect that issue negatively. I will look into it closely.

i have a question though, in the CI it looks like the arm build is failing
https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-7917/7/pipeline/#step-163-log-112
should i try to fix it? please advise.

@tkonolige
Copy link
Contributor

@mherkazandjian I'm sorry it took me so long to remember this, but I don't think this PR will work on macOS. Here is what I had in my version of this:

# On MacOS, the default C compiler (/usr/bin/cc) is actually a small script that dispatches to a
# compiler the default SDK (usually /Library/Developer/CommandLineTools/usr/bin/ or
# /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/). CMake
# automatically detects what is being dispatched and uses it instead along with all the flags it
# needs. CMake makes this second compiler avaliable through the CMAKE_C_COMPILER variable, but it
# does not make the necessary flags available. This leads to configuration errors in libbacktrace
# because it can't find system libraries. Our solution is to detect if CMAKE_C_COMPILER lives in
# /Library or /Applications and switch to the default compiler instead.
if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND (CMAKE_C_COMPILER MATCHES "^/Library"
  OR CMAKE_C_COMPILER MATCHES "^/Applications"))
  set(c_compiler "/usr/bin/cc")
else()
  set(c_compiler "${CMAKE_C_COMPILER}")
endif()

Note that this solution is not quite correct. You probably want to set the sdk path to whatever the user set it to.

@mherkazandjian
Copy link
Contributor Author

btw, I tried these same changes when debugging #7705 but they did not fix it.

hi @tkonolige

@mherkazandjian I'm sorry it took me so long to remember this, but I don't think this PR will work on macOS. Here is what I had in my version of this:

# On MacOS, the default C compiler (/usr/bin/cc) is actually a small script that dispatches to a
# compiler the default SDK (usually /Library/Developer/CommandLineTools/usr/bin/ or
# /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/). CMake
# automatically detects what is being dispatched and uses it instead along with all the flags it
# needs. CMake makes this second compiler avaliable through the CMAKE_C_COMPILER variable, but it
# does not make the necessary flags available. This leads to configuration errors in libbacktrace
# because it can't find system libraries. Our solution is to detect if CMAKE_C_COMPILER lives in
# /Library or /Applications and switch to the default compiler instead.
if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND (CMAKE_C_COMPILER MATCHES "^/Library"
  OR CMAKE_C_COMPILER MATCHES "^/Applications"))
  set(c_compiler "/usr/bin/cc")
else()
  set(c_compiler "${CMAKE_C_COMPILER}")
endif()

Note that this solution is not quite correct. You probably want to set the sdk path to whatever the user set it to.

ok tnx for the update.
Actually the CI is failing for all the builds (see below) not just for arm as i was thinking.
image
i think we need to pass -DMACHINE_NAME to cmake in the CI build scripts too.

@tkonolige
Copy link
Contributor

@mherkazandjian We should not require MACHINE_NAME be passed in as part of the build. The cmake script should check if it is set and then add the correct configure flags.

@mherkazandjian
Copy link
Contributor Author

@mherkazandjian We should not require MACHINE_NAME be passed in as part of the build. The cmake script should check if it is set and then add the correct configure flags.

@tkonolige i fixed the issue with the CI, the CPPFLAGS=${CMAKE_CXX_FLAGS} was the cause, I removed it since
g++ is not used/needed for libbacktrace (it is written in C).

the macOS build seems to be passing
image

following up on your comment #7917 (comment)
you mean including the snippet below in Libbacktrace.cmake (although ) fixes the issue for macOS? what did you mean
by "although not correct?"
i do not have access to a mac.

if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND (CMAKE_C_COMPILER MATCHES "^/Library"
  OR CMAKE_C_COMPILER MATCHES "^/Applications"))
  set(c_compiler "/usr/bin/cc")
else()
  set(c_compiler "${CMAKE_C_COMPILER}")
endif()

@mherkazandjian
Copy link
Contributor Author

mherkazandjian commented Apr 29, 2021

@tqchen it looks like the docker daemon did not start in the CI instance
https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-7917/9/pipeline/259
how can one re-trigger the build? it looks like an issue with the CI machine/vm itself.
( i can just push a dummy commit to re-trigger...)

@areusch
Copy link
Contributor

areusch commented Apr 29, 2021

@mherkazandjian i think the CI is having some trouble now, feel free to push an empty commit to retriever

@hogepodge
Copy link
Contributor

We put images into the https://github.com/tlc-pack/web-data repository.

@mherkazandjian
Copy link
Contributor Author

@hogepodge did you get a chance to look at the changes? i think this PR is good to go (the CI tests pass too)

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

You need a couple of changes to make the macOS workaround work.

cmake/libs/Libbacktrace.cmake Outdated Show resolved Hide resolved
cmake/libs/Libbacktrace.cmake Outdated Show resolved Hide resolved
cmake/libs/Libbacktrace.cmake Show resolved Hide resolved
docs/deploy/index.rst Outdated Show resolved Hide resolved
@tqchen tqchen added the status: need update need update based on feedbacks label May 11, 2021
@tqchen
Copy link
Member

tqchen commented May 11, 2021

gentle ping @mherkazandjian please follow up on the review comments and let us merge soon !

@mherkazandjian
Copy link
Contributor Author

gentle ping @mherkazandjian please follow up on the review comments and let us merge soon !

Hi @tqchen
sorry got distracted with some other work. I'll wrap this up before the end of the week.

@mherkazandjian
Copy link
Contributor Author

@tqchen i finished all the requested changes :)

@tqchen tqchen removed the status: need update need update based on feedbacks label May 24, 2021
Copy link
Contributor

@hogepodge hogepodge left a comment

Choose a reason for hiding this comment

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

Throughout this document, there's a mix of using "we" and "you" to describe the process of building TVM. This should be reconciled, preferably to use the 3rd person. That may be a larger task for another PR. A number of other comments to help clarify language and usage.

@@ -14,14 +14,39 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

# On MacOS, the default C compiler (/usr/bin/cc) is actually a small script that dispatches to a
# compiler the default SDK (usually /Library/Developer/CommandLineTools/usr/bin/ or
Copy link
Contributor

Choose a reason for hiding this comment

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

On MacOS, the default C compiler (/usr/bin/cc) is script that dispatches to the compiler
provided by the default SDK (usually /Library/Developer/CommandLineTools/usr/bin/ or

# needs. CMake makes this second compiler avaliable through the CMAKE_C_COMPILER variable, but it
# does not make the necessary flags available. This leads to configuration errors in libbacktrace
# because it can't find system libraries. Our solution is to detect if CMAKE_C_COMPILER lives in
# /Library or /Applications and switch to the default compiler instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Our solution is to detect if CMAKE_C_COMPILER is installed in

- TVM runtime, which runs on the target devices.

In order to integrate the compiled module, we **do not** need to build entire TVM on the target device. You only need to build the TVM compiler stack on your desktop and use that to cross-compile modules that are deployed on the target device.
In order to integrate the compiled module, we **do not** need to build entire
TVM on the target device. You only need to build the TVM compiler stack on your
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not need to build the entire TVM project on the target device. Rather, we only need to build the TVM compiler stack on a host device and use that to cross-compile modules that can be deployed to a target device.

@tqchen tqchen merged commit 06a466c into apache:main Jun 1, 2021
@tqchen
Copy link
Member

tqchen commented Jun 1, 2021

Thank you @mherkazandjian ! Thanks @tkonolige @hogepodge @areusch @leandron for reviewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libbacktrace does not inherit the CMAKE_C_COMPILER and CMAKE_CXX_COMPILER values of the top level project
7 participants