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

Add helper for cross-compiling Halide generators. #6366

Merged
merged 2 commits into from
Nov 1, 2021

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Oct 29, 2021

Created a new function, add_halide_generator, that helps users write correct cross-compiling builds by establishing the following convention for creating a generator named TARGET:

  1. Define Halide generators and libraries in the same project
  2. Assume two builds: a host build and a cross build.
  3. When creating a generator, check to see if we can load a pre-built version of the target.
  4. If so, just use it.
  5. If not, make sure the full Halide package is loaded and create a target for the generator.
    a. If CMAKE_CROSSCOMPILING is set, then warn the user (the variable is unreliable on macOS) that something seems fishy.
    b. Create export rules for the generator. It creates a package PACKAGE_NAME and appends to its EXPORT_FILE.
    c. Create a custom target also named PACKAGE_NAME for building the generators.
    d. Create an alias ${PACKAGE_NAMESPACE}${TARGET}.
  6. Users are expected to use the alias in conjunction add_halide_library. Users can test the existence of TARGET to determine whether a pre-built one was loaded (and set additional properties if not).
  7. Setting ${PACKAGE_NAME}_ROOT is enough to load pre-built generators.

PACKAGE_NAME is ${PROJECT_NAME}-halide_generators by default.
PACKAGE_NAMESPACE is ${PROJECT_NAME}::halide_generators:: by default.
EXPORT_FILE is ${PROJECT_BINARY_DIR}/cmake/${PACKAGE_NAME}-config.cmake by default.

Users are free to avoid the helper if it would not fit their workflow.

This will be useful to integrate into HANNK. It abstracts the approach in #6365 and will hopefully enable cross-compilation for the other apps, too.

Skipping buildbots because only the test/integration code uses it for now (and only the GHA tests use this)

@alexreinking alexreinking added buildbot_test_nothing Buildbots ignore this PR entirely. Takes precedence over all other buildbot_test_* labels. skip_buildbots Synonym for buildbot_test_nothing labels Oct 29, 2021
@alexreinking alexreinking force-pushed the alex/add_halide_generator branch from c4fa50c to 58af0ac Compare October 29, 2021 02:42
@alexreinking alexreinking force-pushed the alex/add_halide_generator branch from 58af0ac to d4a875d Compare October 29, 2021 02:46
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.

Generally looks good -- relevant comments are mostly on the other PR right now.

Created a new function, `add_halide_generator`, that helps users write correct
cross-compiling builds by establishing the following convention for them:

1. Define Halide generators and libraries in the same project
2. Assume two builds: a host build and a cross build.
3. When creating a generator, check to see if we can load a pre-built version of
   the target.
4. If so, just use it, and add aliases to make sure all the names line up.
5. If not, make sure the full Halide package is loaded and create a target for
   the generator.
     a. If `CMAKE_CROSSCOMPILING` is set, then _warn_ the user (the variable is
        unreliable on macOS) that something seems fishy.
     b. Create export rules for the generator. It creates a package
        `PACKAGE_NAME` and appends to its `EXPORT_FILE`.
     c. Create a custom target also named `PACKAGE_NAME` for building the
        generators.
     d. Create an alias `${PACKAGE_NAMESPACE}${TARGET}`.
6. Users are expected to use the alias in conjunction `add_halide_library`.
   Users can test the existence of `TARGET` to determine whether a pre-built one
   was loaded (and set additional properties if not).
7. Setting `${PACKAGE_NAME}_ROOT` is enough to load pre-built generators.

`PACKAGE_NAME` is `${PROJECT_NAME}-generators` by default.
`PACKAGE_NAMESPACE` is `${PROJECT_NAME}::generators::` by default.
`EXPORT_FILE` is `${PROJECT_BINARY_DIR}/cmake/${PACKAGE_NAME}-config.cmake` by
default.

Users are free to avoid the helper if it would not fit their workflow.
@alexreinking alexreinking force-pushed the alex/add_halide_generator branch from fff7422 to e0ddb92 Compare November 1, 2021 20:33
@steven-johnson
Copy link
Contributor

What needs to happen for this to land -- do we have enough test coverage? Do we need to update apps/hannk to use this rule?

@alexreinking
Copy link
Member Author

What needs to happen for this to land -- do we have enough test coverage? Do we need to update apps/hannk to use this rule?

I have already updated HANNK to use this in a separate branch... would you like me to group these changes together, then?

@steven-johnson
Copy link
Contributor

would you like me to group these changes together, then?

Not necessary as long as it all lands close enough together, do as you see fit for reviewability

@alexreinking alexreinking removed skip_buildbots Synonym for buildbot_test_nothing buildbot_test_nothing Buildbots ignore this PR entirely. Takes precedence over all other buildbot_test_* labels. labels Nov 1, 2021
@alexreinking
Copy link
Member Author

Here, I'm adding the HANNK change so that we can look at how the helper will be used in a real world scenario.

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

@steven-johnson
Copy link
Contributor

basically all good

@alexreinking alexreinking merged commit 4225eba into master Nov 1, 2021
@alexreinking alexreinking deleted the alex/add_halide_generator branch November 1, 2021 23:03
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