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

Use explicit host build strategy for cross compiling HANNK #6365

Merged
merged 18 commits into from
Nov 1, 2021

Conversation

alexreinking
Copy link
Member

There's a bunch of changes in here, and I thought it would be better to review them here, rather than just pushing to your branch. I took some care to make each commit self-contained and easy to interpret. The big changes are in the last two:

First, the cross-compiling strategy changed significantly. Rather than shelling out via execute_process and potentially guessing the toolchain options wrong, expect to find our host tools (i.e. generators) in a package called "hannk_tools".

The package is created by the host build via the CMake export() command. Importing this package in the cross build creates IMPORTED targets with the same names as our generators. We then use these generators rather than creating generators for the target build.

Second, the configure_cmake.sh script now takes charge of creating a host build when cross compiling for Android or WASM.

Oh yeah, Android NDK cross compiling works now, too!

Overall, the code for this is much smaller and more robust to unknown environments.

Rather than shelling out via execute_process and potentially
guessing the toolchain options wrong, expect to find our host
tools (i.e. generators) in a package called "hannk_tools".

The package is created by the host build via the CMake export()
command. Importing this package in the cross build creates
IMPORTED targets with the same names as our generators. We then
use these generators rather than creating generators for the
target build.
@steven-johnson
Copy link
Contributor

the configure_cmake.sh

I told you this sort of thing was handy :-)

@alexreinking
Copy link
Member Author

I told you this sort of thing was handy :-)

Yes, you did and it is! Though I think this could still be a thin wrapper around CMake presets :)

@steven-johnson
Copy link
Contributor

Yes, you did and it is! Though I think this could still be a thin wrapper around CMake presets :)

I'd be ok with that, with the caveat that I find the CMake presets stuff harder to scan and figure out quickly.

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

This took me a few read-throughs -- and some experimentation! -- to fully realize how this worked, so some expanded commenting on the approach is definitely worthwhile. (I realize my original PR was similarly uncommented...)

Anyway, if I am reading this correctly, the approach you propose is that we always make a 'host' build config, but we also export a specific target (hannk_tools in this case) that can be used to build just the subset of stuff that is halide-aot code; when we really are compiling for host, this target is a no-op; when crosscompiling, we make a second cmake build config (for that), which references the export-native stuff via find_package. Is this correct?

This seems pretty slick, and could likely be scaled to work with other apps as well. (And unlike my approach, it doesn't look like it requires a hard 'subdirectory' boundary for cross-compilation, though I'd argue that's usually a good idea anyway.)

The one gotcha with approach that I see right now is that the captive 'host' sub-build we use still has to do FetchContent for stuff that isn't being build or used -- in this case, all of TFLite -- which is expensive and annoying. (Whether using FetchContent for this purpose is a separate issue, of course, but there would likely be other CMake stuff in other apps or use cases that would be desirable to subvert.) On the assumption that this sort of this is uncommon, though, maybe it would suffice to have a convention for some CMake definition that could be used to skip the FetchContent? (e.g., -DHALIDE_AOT_HOST_ONLY=ON when doing the 'captive' host build, which expensive & irrelevant parts of the CMake file could sniff for to save time?)

apps/hannk/CMakeLists.txt Show resolved Hide resolved
apps/hannk/CMakeLists.txt Show resolved Hide resolved
apps/hannk/CMakeLists.txt Show resolved Hide resolved
apps/hannk/configure_cmake.sh Outdated Show resolved Hide resolved
apps/hannk/configure_cmake.sh Outdated Show resolved Hide resolved
apps/hannk/halide/CMakeLists.txt Outdated Show resolved Hide resolved
apps/hannk/halide/CMakeLists.txt Outdated Show resolved Hide resolved
apps/hannk/halide/CMakeLists.txt Outdated Show resolved Hide resolved
apps/hannk/halide/CMakeLists.txt Show resolved Hide resolved
apps/hannk/halide/CMakeLists.txt Outdated Show resolved Hide resolved
@alexreinking
Copy link
Member Author

This took me a few read-throughs -- and some experimentation! -- to fully realize how this worked, so some expanded commenting on the approach is definitely worthwhile. (I realize my original PR was similarly uncommented...)

Anyway, if I am reading this correctly, the approach you propose is that we always make a 'host' build config,

Yep!

but we also export a specific target (hannk_tools in this case) that can be used to build just the subset of stuff that is halide-aot code; when we really are compiling for host, this target is a no-op;

This is almost right... what we're exporting are the AOT generators. It's a no-op for the cross-build, not for the host.

when crosscompiling, we make a second cmake build config (for that), which references the export-native stuff via find_package. Is this correct?

This is exactly right.

The one gotcha with approach that I see right now is that the captive 'host' sub-build we use still has to do FetchContent for stuff that isn't being build or used -- in this case, all of TFLite -- which is expensive and annoying.

Yeah, I did already notice this. I'm surprised it takes so long to download.

On the assumption that this sort of this is uncommon, though, maybe it would suffice to have a convention for some CMake definition that could be used to skip the FetchContent? (e.g., -DHALIDE_AOT_HOST_ONLY=ON when doing the 'captive' host build, which expensive & irrelevant parts of the CMake file could sniff for to save time?)

I think this is a good idea.

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

OK, I think we just need a few comment changes/augmentations, addition of HALIDE_AOT_HOST_ONLY, and (maybe) the renaming of hannk_tools to something else, and we'll be good to go. (I can make these changes but won't do so unless you want me too, since this is already a PR-on-a-PR)

@alexreinking
Copy link
Member Author

@steven-johnson - done. PTAL. Still TODO is improving the "ninja in the cross build directory only" workflow.

@steven-johnson steven-johnson added the buildbot_test_everything Buildbots should run all available tests on this PR (unless build_test_nothing is set). label Nov 1, 2021
Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting on green buildbots

@steven-johnson
Copy link
Contributor

actually, nm, since this is merging into another PR, we'll merge it and do the buildbot tests there

@steven-johnson steven-johnson merged commit dae7c4e into srj/hannk-wasm-build Nov 1, 2021
@steven-johnson steven-johnson deleted the alex/hannk-wasm-build branch November 1, 2021 18:41
steven-johnson added a commit that referenced this pull request Nov 1, 2021
* [hannk] Allow disabling TFLite+Delegate build in CMake

Preparatory work for allowing building of hannk with Emscripten; TFLite (and its dependees) problematic to build in that environment, but this will allow us to build a tflite-parser-only environment.

(Note that more work is needed to get this working for wasm, as crosscompiling in CMake is still pretty painful; this work was split out to make subsequent reviews simpler)

* [hannk] Add support for building/running for wasm

* HANNK_BUILD_TFLITE_DELEGATE -> HANNK_BUILD_TFLITE

* Use explicit host build strategy for cross compiling HANNK (#6365)

* Ignore local emsdk clone

* Fix usage of CMAKE_BUILD_TYPE

* Only print the Halide target info once per CMake run

* Fix Halide "cmake" target detection for Emscripten

* Prefer target_link_options to _link_libraries when applicable

* Validate, rather than find, NODE_JS_EXECUTABLE (set by emsdk)

* Emscripten already wraps tests with node.

* Add dependency on Android logging library.

* For cross-compiling, find host tools instead of recursive call.

Rather than shelling out via execute_process and potentially
guessing the toolchain options wrong, expect to find our host
tools (i.e. generators) in a package called "hannk_tools".

The package is created by the host build via the CMake export()
command. Importing this package in the cross build creates
IMPORTED targets with the same names as our generators. We then
use these generators rather than creating generators for the
target build.

* Rework cross-compiling script.

* Respond to (easy) reviewer comments.

* Add HANNK_AOT_HOST_ONLY option. Use in script.

* [hannk] tests should only process .tflite files (#6368)

currently, random dotfiles (e.g. .DS_Store on OSX) can creep in, causing bogus failures

* Add comment about node wrapping.

* Rename hannk_tools to hannk-halide_generators

* Add comment about exporting targets.

* Bump version to Halide 14.0.0 (#6369)

Co-authored-by: Steven Johnson <srj@google.com>

Co-authored-by: Alex Reinking <alex_reinking@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildbot_test_everything Buildbots should run all available tests on this PR (unless build_test_nothing is set).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants