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

Updated Dxc integration for DX12 backend #3356

Merged
merged 33 commits into from
Jan 18, 2023

Conversation

Elabajaba
Copy link
Contributor

@Elabajaba Elabajaba commented Jan 6, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.
  • Figure out how we should enable and disable DXC, and whether that's at runtime/compiletime/startup/etc
  • Figure out what the fallback should be if the user enables DXC, but the .dlls aren't available (crash with an error message? Fall back to fxc but log the error)
  • Decide if we're going to allow users to set an explicit path to the .dll location
  • Figure out why it's working on my machine if I only have dxil.dll, and not dxcompiler.dll in the folder I have the Windows SDK installed
  • Documentation about why users would/should use DXC, and how to get the .dlls
  • Fix all the tests in github actions being broken because the .dlls are missing Currently defaults to using FXC.

Connections
Closes #2722
Builds off of, and closes #3147

Description
wgpu currently uses the old, slow, unmaintained FXC shader compiler for DX12. This allows use of Microsoft's newer faster shader compiler and also allows use of ShaderModel 6.0 instead of being limited to SM 5.1.

Testing
Tested both the FXC and DXC paths by running cargo nextest, as well as running the examples.

@cwfitzgerald cwfitzgerald mentioned this pull request Jan 6, 2023
3 tasks
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.

Some comments:

Figure out why it's working on my machine if I only have dxil.dll, and not dxcompiler.dll in the folder

You probably have the windows sdk, which includes dxc.

Figure out how we should enable and disable DXC

I think we should have a feature at the wgpu-hal level, but not at the wgpu level. wgpu the crate unconditionally depends on it.

Figure out what the fallback should be if the user enables DXC, but the .dlls aren't available (crash with an error message? Fall back to dxc but log the error?)

We should warn in the case the user wants dxc, but the path isn't found, then fall back to FXC.

Figure out how we should enable and disable DXC, and whether that's at runtime/compiletime/startup/etc

I'm thinking of this enum sitting in an InstanceDescriptor:

enum Dx12Compiler {
    #[default]
    Fxc,
    Dxc { dxil_path: Option<String>, dxc_path: Option<String> },
}

Fix all the tests in github actions being broken because the .dlls are missing

We likely should default our tests to be fxc only, but with a flag they can enable DXC support. FF etc will be using fxc, so it's the most important path.

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023

Codecov Report

Merging #3356 (2048c27) into master (0849e78) will increase coverage by 0.26%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master    #3356      +/-   ##
==========================================
+ Coverage   65.23%   65.49%   +0.26%     
==========================================
  Files          86       66      -20     
  Lines       42772    37420    -5352     
==========================================
- Hits        27901    24508    -3393     
+ Misses      14871    12912    -1959     
Impacted Files Coverage Δ
player/src/bin/play.rs 0.65% <0.00%> (-0.02%) ⬇️
wgpu-hal/src/lib.rs 24.03% <ø> (-2.89%) ⬇️
wgpu/src/context.rs 53.32% <ø> (ø)
wgpu/src/util/init.rs 68.25% <61.53%> (-1.75%) ⬇️
wgpu-core/src/hub.rs 64.90% <100.00%> (+3.77%) ⬆️
wgpu-core/src/instance.rs 68.17% <100.00%> (+2.09%) ⬆️
wgpu-info/src/main.rs 76.41% <100.00%> (ø)
wgpu-types/src/lib.rs 86.59% <100.00%> (-0.86%) ⬇️
wgpu/src/backend/direct.rs 56.90% <100.00%> (-0.09%) ⬇️
wgpu/src/lib.rs 51.30% <100.00%> (+0.26%) ⬆️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Elabajaba Elabajaba marked this pull request as ready for review January 12, 2023 07:56
@Elabajaba Elabajaba requested a review from crowlKats as a code owner January 12, 2023 07:56
Copy link
Contributor Author

@Elabajaba Elabajaba left a comment

Choose a reason for hiding this comment

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

A few changes, and a reminder for myself that I need to retest this on my laptop.

wgpu-hal/src/dx12/adapter.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu/src/util/init.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
player/src/bin/play.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.

This is really shaping up well. Got some comments, but nothing show stopping!

wgpu-core/src/instance.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/util/init.rs Outdated Show resolved Hide resolved
wgpu/src/util/init.rs Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/shader_compilation.rs Outdated Show resolved Hide resolved
@Elabajaba
Copy link
Contributor Author

I think everything should be cleaned up now.

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.

Amazing stuff!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) January 18, 2023 21:15
@cwfitzgerald cwfitzgerald merged commit 81569dd into gfx-rs:master Jan 18, 2023
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.

DXC Integration in DX12 Backend
5 participants