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

Support CMAKE_OSX_ARCHITECTURES #8390

Merged
merged 20 commits into from
Sep 4, 2024
Merged

Support CMAKE_OSX_ARCHITECTURES #8390

merged 20 commits into from
Sep 4, 2024

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Aug 12, 2024

As requested at the last Halide dev meeting, I have drafted an implementation of support for CMAKE_OSX_ARCHITECTURES. This is again quite complicated, in part because I attempted to avoid breaking existing user code.

As it stands, multi-target alternative selection and feature specification are stepping on each others' toes.

I'll write more tomorrow.

Fixes #8134


Breaking suggestion:

  • The TARGETS argument and the Halide_TARGET variable shall take only the following values:
    • host (passed verbatim to the generator, but resolved for feature selection)
    • cmake
    • ARCH-BITS-OS (a triple)
  • When CMAKE_OSX_ARCHITECTURES is set, cmake expands to a list of compatible architectures, e.g. arm-64-osx;x86-64-osx. This is reflected in Halide_CMAKE_TARGET. host expands to a single triple corresponding to the architecture of the cmake executable itself (this enables compatibility with the arch command).
  • The FEATURES argument applies a list of features to ALL alternatives on ALL platforms (useful for, e.g. user_context, name mangling, etc.)
  • The FEATURES[platform] arguments set up a list of alternatives (e.g. FEATURES[x86-64-linux] "avx2-sse4" "sse4") for a particular triple.

What this doesn't resolve is how to do things like TARGETS cmake-no_bounds_query cmake effectively. Perhaps the following adjustments to the above help:

  • Rename FEATURES[platform] to MULTITARGET[platform]
  • Add MULTITARGET (unadorned) to provide the default value for unspecified [platform]s.

Specifying an empty entry in FEATURES/MULTITARGET or whatever is challenging in CMake because it really wants to drop empty function arguments. We might need to add a dummy feature like none (either in the actual Generator or just in the build system) that does nothing but isn't the empty string.

@alexreinking alexreinking added release_notes For changes that may warrant a note in README for official releases. dev_meeting Topic to be discussed at the next dev meeting labels Aug 12, 2024
"COMMAND;DEPENDS;EXTRA_OUTPUTS;PARAMS;PLUGINS;TARGETS"
)

## "hash table" of extra outputs to extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add 'hlpipe' here too? It should already be hooked up in the generator interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. Didn't know it was missing!

OUTPUT_VARIABLE merged_libs_targets)

find_program(LIPO lipo REQUIRED)
add_custom_command(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done, and with all the dependencies setup too!

add_halide_library(adams2019_train_cost_model FROM adams2019_cost_model.generator
GENERATOR train_cost_model
FUNCTION_NAME train_cost_model
TARGETS cmake
FEATURES[x86-64-osx] avx2 sse41
FEATURES[arm-64-osx] arm_dot_prod-arm_fp16
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these features available on all Apple Silicon?

@alexreinking alexreinking force-pushed the build/osx-universal2 branch 2 times, most recently from 3f4eb33 to 88701db Compare August 14, 2024 14:29
Comment on lines -25 to +28
function(_bundle_static_is_apple_libtool result item)
function(_bundle_static_is_apple_libtool result_var item)
_bundle_static_check_output(version_info "${item}" -V)
if (version_info MATCHES "Apple, Inc.")
set(result 1 PARENT_SCOPE)
else ()
set(result 0 PARENT_SCOPE)
if (NOT version_info MATCHES "Apple, Inc.")
set(${result_var} 0 PARENT_SCOPE)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a drive-by fix.

@alexreinking
Copy link
Member Author

Topics for discussion at the dev meeting:

  1. How to test this on CI?
  2. How to test build sanity on iOS?

@alexreinking alexreinking marked this pull request as ready for review August 14, 2024 21:30
@alexreinking
Copy link
Member Author

alexreinking commented Aug 14, 2024

Finally, we have a clean build! The halide-asan test failure is due to an LLVM build bug that was fixed in a revert five days ago. Marking this as Ready for Review.

@derek-gerstmann -- I'd appreciate some help testing offline.

@derek-gerstmann
Copy link
Contributor

Testing on my MacBook Pro M1 Max and everything appears to work! I agree, it'd be nice to find a testing strategy for both desktop and mobile.

@alexreinking
Copy link
Member Author

alexreinking commented Aug 16, 2024

From dev meeting:

  • Need to collect multiple extra-outputs in the output variables (how to test?)
  • Need to provide a way to generate the c sources as a byproduct, rather than going through the configured compiler
  • Testing: have ARM builders be fat. Test with Rosetta, too.

@alexreinking
Copy link
Member Author

This is ready on my end. @steven-johnson -- we should coordinate adding testing to the buildbots. Should we wait to land this until we have that?

@steven-johnson
Copy link
Contributor

This is ready on my end. @steven-johnson -- we should coordinate adding testing to the buildbots. Should we wait to land this until we have that?

yes, probably

@alexreinking alexreinking changed the title WIP Support CMAKE_OSX_ARCHITECTURES Support CMAKE_OSX_ARCHITECTURES Aug 29, 2024
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.

Is there documentation/README for this anywhere...?

@alexreinking
Copy link
Member Author

Is there documentation/README for this anywhere...?

No, not yet

@alexreinking
Copy link
Member Author

Landing this with the caveat that it might need adjustment to support IOS cross-compilation via Xcode. At the moment it seems to have not broken existing workflows and has (locally) enabled universal builds on macOS.

@alexreinking alexreinking merged commit b3c8c8b into main Sep 4, 2024
19 checks passed
@alexreinking alexreinking deleted the build/osx-universal2 branch September 4, 2024 00:53
mcourteaux pushed a commit to mcourteaux/Halide that referenced this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev_meeting Topic to be discussed at the next dev meeting release_notes For changes that may warrant a note in README for official releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Halide_BUNDLE_LLVM doesn't work with universal builds on OSX
3 participants