Replace usage of spirv-* binaries with spirv-tools rust crate

[Jake-Shadle]

Making this a draft so that others can leave comments, I think I need to add a wrapper around the Validator as otherwise we will still require having spirv-tools installed which is kind of lame when we are already paying the compile time cost for building spirv-tools

Additions

• Adds spirv-headers and spirv-tools as git submodules (will add docs about retrieiving/updating them for contributors)
• Adds spirv-tools-sys which builds spirv-tools, including a small generator script that generates some includes based on spirv-headers
• Adds a spirv-tools-sys/src/c/opt.cpp which is just a thin (and incomplete) C wrapper around Optimizer because C++
• Adds a spirv-tools crate which provides a slighly nicer API on top of spirv-tools-sys

Changes

• Replaces the usage of the spirv-opt binary in link.rs with spirv_tools::opt::Optimizer
• Replaces the usage of spirv-val for validating resulting binaries in link.rs with spirv_tools::val::Validator
• Replaces the usage of spirv-as in the linker tests with spirv_tools::as::Assembler
• All of the crates that use rustc_codegen_spirv now have a use-installed-tools and use-compiled-tools feature flags, where use-compiled-tools is the default feature for optimal user experience.

Missing

• There is one change I had to do in our spirv-tools fork due some incredible strangeness with libstdc++ streams, so right now the Assembler will choke on hexadecimal floats, but we only use the Assembler in tests and none of them currently use hexadecimal floats, so figured this would be ok for now, the issue is fixable but doing C++ code was making me sad so I punted until it became an actual issue.

Resolves: #31

[XAMPPRocky]

Personally I would prefer if we could avoid submodules, they are a pain to maintain, and I would even say they're anti-pattern. I think it would be a lot better if we could use git subtree for pulling in those repositories instead. They're newer and much easier to maintain as it prevents a lot of foot guns of updating submodules and keeping the repository in sync with them.

Subtree docs

• https://github.com/git/git/blob/master/contrib/subtree/git-subtree.txt
• https://www.atlassian.com/git/tutorials/git-subtree

[khyperia]

oh no, personally I hate git subtree and would much prefer submodules :P

[Jasper-Bekkers]

+1 to submodules.

// The spirv tools use generated code, for now we just replicate the minimum
// generation we need here by calling the *shudders* python script(s) we need
// to in a simple script and commit them to source control, as they only need
// to be regenerated when spirv-headers is updated 

This deserves a shout-out here (or anywhere) since this adds a new manual step.

Is there any way we can keep this SPIRV-Headers and the one used by rspirv in sync easily other then manually checking hashes?

[XAMPPRocky]

@khyperia Can I ask the motivation behind that? In my experience submodules are hard to use right, and more work to maintain. Every contributor has to keep their submodules in their local version in sync individually, and you can end up in a state where new changes in the submodule break your code, and vice versa but you don't find out until after you have landed changes. With git subtree it's a lot easier to ensure that you can always build your codebase, and makes so that most contributors don't have to care about it since the only time a contributor would need to sync is when they want to update spirv-tools, and they would also have to ensure that the code continues to pass the tests. From the git docs:

Unlike submodules, subtrees do not need any special constructions (like .gitmodules files or gitlinks) be present in your repository, and do not force end-users of your repository to do anything special or to understand how subtrees work. A subtree is just a subdirectory that can be committed to, branched, and merged along with your project in any way you want.

[Jake-Shadle]

This deserves a shout-out here (or anywhere) since this adds a new manual step.

Yah I will add docs for this. To be clear, all the generated files are in checked in in spirv-tools-sys/generated so they aren't built every time, but yes, if spirv-headers is changed you will need python etc.

Is there any way we can keep this SPIRV-Headers and the one used by rspirv in sync easily other then manually checking hashes?

Good question, we can at minimum assert in the build script if their commits differ, or maybe, just the files that we read are the same but...yah.

[Jake-Shadle]

So good news, spirv-opt and spirv-val are now compiled as crates and can be run without installing spirv-tools, yay!
...bad news, extremely slow to compile, oh and I manage to get an exception that seems particular to libstdc++ on both my local machine and the ubuntu runner...

So not sure how we want to handle this.

1. We could install spirv-tools on CI only, and just feature toggle the spirv-tools crate off so we don't take the compile hit, but this introduces a really big diff between the local and CI that would potentially be draining and annoying.
2. We could move our CI to buildkite and throw more cores/sccache etc at it, as it shouldn't really take that long to build (1m11s on my machine), but that has the drawback of being a little more overhead for us and contributors since it's not a common CI, though I don't think that's a strong argument as the important green = good, red = bad, here are some logs are all the same

[repi]

Would prefer to stay with GitHub Actions for now at least, it is more accessible for users and has a wider ecosystem and easier if anyone forks or tries out things and this is a very open project so that is a priority.

Still need decent CI build times though (#107) as we are working on a lot of different areas through small PRs here. Think it would be fine to have CI just download the executables, esp. if it saves massive amount of time. Users likely will want to be able to run locally with pre-built or their own tools as well, but good to have a default that builds everything (if it doesn't take forever)

[repi]

Long term wanted future: Rust native versions of esp. the parts of spirv-tools we use. Not sure how realistic

[Jake-Shadle]

Think it would be fine to have CI just download the executables, esp. if it saves massive amount of time. Users likely will want to be able to run locally with pre-built or their own tools as well, but good to have a default that builds everything (if it doesn't take forever)

So I think maybe we can make it slightly less terrible but having a feature flag on spirv-tools for "use executables" vs "use compiled sys crate" so the interface would be the same, just internally it would use one or the other, so would greatly lessen the pain of maintenance, at least for a while.

Long term wanted future: Rust native versions of esp. the parts of spirv-tools we use. Not sure how realistic

Given the commit frequency (though most of the recent ones seem focused on spirv-fuzz which I don't know if we have any interest in) that might be tough, not necessarily to port what is currently there, but to keep up with features, new versions etc.

[repi]

feature flag on spirv-tools for "use executables" vs "use compiled sys crate"

Agreed, that would likely be a decent compromise

And yeah also think it would hard to keep up with spirv-tools in general for a Rust port. But if we primarily use it to optimize the spirv that is a much smaller scope. fuzzing and validation are more dev features that are needed now but shouldn't be needed by other devs that are using Rust GPU once it is more mature. I think.

[Jake-Shadle]

Yah, I guess the good thing is that right now the way the spirv-tools optimizer is structured in passes, it might not be too bad to port passes as needed, and since we control the build system, pick the rust pass or the c++ pass, though I haven't looked a ton at how much data/state is shared between passes (hopefully little).

[khyperia]

And yeah also think it would hard to keep up with spirv-tools in general for a Rust port. But if we primarily use it to optimize the spirv that is a much smaller scope. fuzzing and validation are more dev features that are needed now but shouldn't be needed by other devs that are using Rust GPU once it is more mature. I think.

Some context, we decided in #32 to always run spirv-val on user's machines, and have no plans to stop doing so for a long time.

[repi]

Some context, we decided in #32 to always run spirv-val on user's machines, and have no plans to stop doing so for a long time.

Ah good to know, and makes sense, esp at this early stage.

Would ultimately want users to be able to build shaders without any external C/C++ dependencies or system executables, but that can be a larger target as part of long term design of #48 and potentially other separate issues

[Jake-Shadle]

Ok, I've replaced all of the usages of spirv-* binaries, now I just need to add them back behind a feature toggle for the spirv-tools crate so that we can just use those in CI, but make them an optional requirement for users.

[XAMPPRocky]

Another potential footgun with submodules that doesn't matter much now, is that having different submodules checked out doesn't count as "dirty" to git/cargo so you could publish accidentally publish the wrong commit. Here's steps how to replicate an example of how to that.

1. Change the branch in spirv-tools's .gitmodules to master.
2. Run git submodules update --remote
3. Run cargo package

[Jake-Shadle]

I've updated the top of the PR with the one remaining TODO of fixing up documentation, but everything else should be mostly done.

[khyperia]

Holy heck this is a lot, thank you so much for doing this! This is pseudo-approval, but I don't want mergify to shove this in before you have the chance to check optional feedback.

[Jake-Shadle]

Oh, one thing I forgot to ask, should this PR making running the Optimizer on by default? I just kept the logic of it being gated behind an environment variable, but that seems dangerous since it doesn't get CI coverage then.

[khyperia]

Oh, one thing I forgot to ask, should this PR making running the Optimizer on by default? I just kept the logic of it being gated behind an environment variable, but that seems dangerous since it doesn't get CI coverage then.

In the long-term, I think so, however, it gets a little in-depth - e.g. we want rustc_codegen_spriv tests (currently located at spirv-builder/src/test/*) to not run spirv-opt. I thiiink it'd be nicest to do all that configuration plumbing after this PR, since this is already really big.

[Jake-Shadle]

Very true! Ok, should I just update the docs in this PR and we'll call it done and merge it in?

[khyperia]

yeah!

[Jake-Shadle]

Encountered an issue with Windows that I had missed because the tests weren't running where either the use of stdin for input or stdout for output of the program caused problems for the spirv-tools, so until I look into that deeper I just returned to using files for the input and output just so I can get this PR landed.