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

Static linking for DXC via mach-dxcompiler #6574

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

DouglasDwyer
Copy link
Contributor

@DouglasDwyer DouglasDwyer commented Nov 21, 2024

Connections

Description
This PR adds support for statically linking the DXC binaries from mach-dxcompiler. The mach-dxcompiler repo replaces Microsoft's DXC build system with Zig, which allows for building DXC as a static .lib file. I have created an upstream crate called mach-dxcompiler-rs that handles downloading a prebuilt mach-dxcompiler binary and linking it into a Rust project (ideally, it would be built from source, but that was too complicated for my purposes - it can be added later if desired).

This PR adds a dependency on mach-dxcompiler-rs, gated behind the mach-dxcompiler-rs feature flag. When enabled, it allows for specifying dx12_shader_compiler: Dx12Compiler::MachDxc in the InstanceDescriptor. This allows for using DXC in WGPU without having to ship any external .dlls alongside the main binary.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • (n/a) --target wasm32-unknown-unknown
    • (n/a) --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@DouglasDwyer DouglasDwyer requested a review from a team as a code owner November 21, 2024 03:17
@DouglasDwyer
Copy link
Contributor Author

Update on the current state of this PR:

The branch builds and I am able to successfully use static DXC in my own WGPU project on windows-msvc. However, the Windows WGPU unit tests don't compile due to a linking error: the mach-dxcompiler lib is built with MT_StaticRelease whereas another native library referenced by WGPU (nv-flip-sys) is built with MD_DynamicRelease. In general, I believe that this would be an error for any consumers of WGPU + Mach DXC that also dynamically link MSVCRT from other native libs.

From what I gather, the correct solution here is to offer both an MD_DynamicRelease and MT_StaticRelease binary for the the mach-dxcompiler Windows target. I played with the Zig build system for about four hours yesterday, and was unable to figure out how to emit a .lib file that dynamically links MSVCRT. It looks to me like the choice between static/dynamic CRT is hardcoded in the Zig build system, and is tied directly to whether you are building a .lib or .dll.

I will be asking around on the Zig forums (and maybe opening an issue on Zig's GitHub) about this. In the meantime, I will leave this PR open. If anyone has additional guidance or suggestions for how we can push this PR forward, I would be grateful. Otherwise, this PR is stagnant until we find a way to build static libs with MD_DynamicRelease using Zig.

@DouglasDwyer
Copy link
Contributor Author

DouglasDwyer commented Nov 22, 2024

Well, after a good deal of more foraging, I managed to find the compiler flag that I was looking for: Zig accepts -fms-runtime-lib=dll to use the dynamic MSVCRT, just like Clang. With this, I was able to fix the linking issues and update mach-dxcompiler-rs to support either static/dynamic MSVCRT. All tests now build and I am able to build/run WGPU + Mach DX12 in my own project. This PR should be ready for review.

@Elabajaba
Copy link
Contributor

I'm just curious, do you know if mach-dxcompiler is keeping up to date with DXC? From what I remember from the original blog post they didn't plan to try to track upstream at the time, but there's been some big features added in shader model 6.8 (workgraphs being the obvious headliner, but SV_StartVertex/InstanceLocation is also really nice especially for simplifying GPU driven rendering pipelines.

@DouglasDwyer
Copy link
Contributor Author

@Elabajaba, that's a great question. The DXC branch that's actually being built is here - it hasn't been updated in about a year, so it doesn't support SM 6.8 at this time. In theory it should be possible to update it to latest, but I took a look and there are merge conflicts with DXC master. I don't know how easy it would be.

@teoxoy
Copy link
Member

teoxoy commented Nov 27, 2024

I saw that mach-dxcompiler signs the blob but I don't know if that works if we pass it DXC_ARG_SKIP_VALIDATION (which we currently do). We should probably remove the flag if the validator is None.

@teoxoy
Copy link
Member

teoxoy commented Nov 27, 2024

Other than that, this looks good to me, @cwfitzgerald what do you think?

@teoxoy
Copy link
Member

teoxoy commented Nov 27, 2024

If we get this in we can probably close #4155.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Broadly looks good, some notes.

wgpu/Cargo.toml Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Great stuff, one last nit, then let's land!

wgpu/src/util/init.rs Show resolved Hide resolved
@cwfitzgerald cwfitzgerald merged commit be02606 into gfx-rs:trunk Dec 2, 2024
27 checks passed
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.

[Experiment]: bundled with dxc (as a feature?)
4 participants