search

halide / Halide

a language for fast, portable data-parallel computation
https://halide-lang.org
Other
5.77k stars 1.07k forks source link

Use upstream interface for consuming SPIR-V #8265

Closed alexreinking closed 2 weeks ago

alexreinking commented 4 weeks ago

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.

alexreinking commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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?)