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 upstream interface for consuming SPIR-V #8265

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Jun 5, 2024

As I want to resume work on our Python packaging, an important prerequisite is auditing our dependencies and the way we consume them / ask downstreams to consume the public subset.

Our vendored SPIR-V headers had diverged from upstream practice for no particular reason. This PR replaces Halide::SPIRV with the upstream SPIRV-Headers::SPIRV-Headers and uses find_package on the upstream CMake package files. This additionally makes it possible for a third-party to easily override the vendored headers.

I also include a script for automatically updating the SPIRV-Headers in the future.

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... but how will this work for Make users? :-/

CMakePresets.json Show resolved Hide resolved
@alexreinking
Copy link
Member Author

LGTM... but how will this work for Make users? :-/

The relevant part of the Makefile is here:

SPIRV_CXX_FLAGS=$(if $(WITH_SPIRV), -DWITH_SPIRV -isystem $(ROOT_DIR)/dependencies/spirv/include, )
SPIRV_LLVM_CONFIG_LIB=$(if $(WITH_SPIRV), , )

The changes in this PR shouldn't break the Make build.

@abadams
Copy link
Member

abadams commented Jun 5, 2024

Is this a correct understanding: The headers still seem to be there, just renamed, so no Makefile changes are necessary. This just adds a feature for cmake users to use a different version from upstream, and also adds a script to update our vendored headers.

@alexreinking
Copy link
Member Author

alexreinking commented Jun 5, 2024

Is this a correct understanding: The headers still seem to be there, just renamed, so no Makefile changes are necessary. This just adds a feature for cmake users to use a different version from upstream, and also adds a script to update our vendored headers.

Yes, though I would argue that the headers are _un_renamed; they had been put into a 1.6 directory, but upstream only uses 1.0, 1.1, and 1.2. From version 1.3 on, they're in unified1.

@derek-gerstmann
Copy link
Contributor

LGTM! Thanks for cleaning things up! There was a lot of churn in the Khronos repo when I was updating the SPIR-V backend. I guess my only concern is that if third-parties wish to override the headers, that this may introduce incompatibilities or a mismatch with the Vulkan runtime.

@alexreinking
Copy link
Member Author

Based on my understanding of the unified1 format, we should be good to go for the future; the definitions we rely on should be stable until there is a major version bump (unified2?)

@alexreinking alexreinking merged commit ea775cc into main Jun 20, 2024
19 checks passed
@alexreinking alexreinking deleted the build-proposals branch June 20, 2024 15:59
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.

4 participants