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

Naga features for platform dependent compilation of msl & hlsl #5919

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jul 6, 2024

Connections

Description

Adds cfg_alias indirection to Naga's x_out features which allows us to introduce new features for platform dependent compilation. This in turn allows wgpu-hal to request shading language output more conditionally.

Drawbacks:

  • build.rs also on Naga
  • new feature names are kinda clunky

Testing
Eyeballing on Windows. Single sample of cargo clean && cargo build -p wgpu --timings looked even more promising than I would have imagined:

Before:
image

After:
image

1.4s cpu time saved on this setup for not actually touching anything 😮. When building wgpu this is even on the hot path, meaning the the wgpu build as a whole got that much faster!
Edit: as expected lots of noise here. Running again on trunk I got 7.0s and 6.2s here, so above trunk compilation is likely with cold file caches. ymmv.

TODO:

  • changelog entry
  • test this manually on mac or linux and report impact

Checklist

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

@Wumpf Wumpf requested review from a team as code owners July 6, 2024 10:10
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

I thought the backends won't be enabled if they won't be used on the target but it seems not because the conditional target compilation is setup in wgpu-core and happens after feature selection. It doesn't feel right that naga gets those new cfgs though; I would say that wgpu-hal is at fault here for not conditionally enabling naga's features based on the target. We already have some target cfgs in wgpu-hal/Cargo.toml, I think we should add naga in those instead of adding new features to naga. Thoughts?

@Wumpf
Copy link
Member Author

Wumpf commented Jul 8, 2024

I would say that wgpu-hal is at fault here for not conditionally enabling naga's features based on the target.

agreed, if possible we should solve it on that level instead 🤔
The problem though is that it's not a pure platform conditional: If I build on Windows without dx12 I expect HLSL to be disabled. If we were to put the naga dependency with hlsl-out under a windows target conditional, it would always enable hlsl-out though even the user left didn't specify dx12.
Putting the hlsl-out feature inside dx12 = [....] gets us back to status quo where we would enable hlsl-out if a user on linux/mac enabled dx12.
Any ideas how to get around this?

@teoxoy
Copy link
Member

teoxoy commented Jul 8, 2024

Any ideas how to get around this?

wgpu-hal/Cargo.toml has some deps behind target cfgs, I thought we could move the naga features under those as well.

@Wumpf
Copy link
Member Author

Wumpf commented Jul 8, 2024

it works if a feature should be dragged in or not on a per-target basis. The problem here is that it's a combination of feature plus target condition that we want this to be active upon. I.e. what we want is:

iff BOTH feature=dx12 is active AND target=windows enable naga's hlsl-out

The left hand side is easy since cargo supports that directly: my_feature = [other_crate/feature]
The right hand side is harder but doable with deps behind target cfg. E.g. khronos-egl is used with different features depending on the platform in wgpu-hal

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
khronos-egl = { version = "6", features = ["dynamic"], optional = true }
#...
[target.'cfg(target_os = "emscripten")'.dependencies]
khronos-egl = { version = "6", features = ["static", "no-pkg-config"] }

The thing I'm struggling here with is to evaluate both conditions at the same time

@teoxoy
Copy link
Member

teoxoy commented Jul 8, 2024

Ah yeah, that's problematic. I previously didn't fully grasp what you meant in your previous comment (#5919 (comment)).

I can't come up with a better solution either. I will take another look at this tomorrow.

@teoxoy teoxoy merged commit 7c51bb4 into gfx-rs:trunk Jul 9, 2024
27 checks passed
@Wumpf Wumpf deleted the naga-platform-conditional-build branch July 9, 2024 10:05
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Jul 30, 2024
Rather than `feature = "blah"`, use the new `cfg` identifiers
introduced by the `cfg_aliases` invocation in `naga/build.rs` to
decide whether to compile the `naga::back::continue_forward` module,
which is only used by the GLSL and HLSL backends.

The `hlsl_out` `cfg` identifer has a more complex condition than just
`feature = "hlsl-out"`, introduced by gfx-rs#5919.
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Jul 30, 2024
Rather than `feature = "blah"`, use the new `cfg` identifiers
introduced by the `cfg_aliases` invocation in `naga/build.rs` to
decide whether to compile the `naga::back::continue_forward` module,
which is only used by the GLSL and HLSL backends.

The `hlsl_out` `cfg` identifer has a more complex condition than just
`feature = "hlsl-out"`, introduced by gfx-rs#5919.

Fixes gfx-rs#6063.
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Jul 31, 2024
Rather than `feature = "blah"`, use the new `cfg` identifiers
introduced by the `cfg_aliases` invocation in `naga/build.rs` to
decide whether to compile the `naga::back::continue_forward` module,
which is only used by the GLSL and HLSL backends.

The `hlsl_out` `cfg` identifer has a more complex condition than just
`feature = "hlsl-out"`, introduced by gfx-rs#5919.

Fixes gfx-rs#6063.
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Jul 31, 2024
Rather than `feature = "blah"`, use the new `cfg` identifiers
introduced by the `cfg_aliases` invocation in `naga/build.rs` to
decide whether to compile the `naga::back::continue_forward` module,
which is only used by the GLSL and HLSL backends.

The `hlsl_out` `cfg` identifer has a more complex condition than just
`feature = "hlsl-out"`, introduced by gfx-rs#5919.

Fixes gfx-rs#6063.
ErichDonGubler pushed a commit that referenced this pull request Jul 31, 2024
Rather than `feature = "blah"`, use the new `cfg` identifiers
introduced by the `cfg_aliases` invocation in `naga/build.rs` to
decide whether to compile the `naga::back::continue_forward` module,
which is only used by the GLSL and HLSL backends.

The `hlsl_out` `cfg` identifer has a more complex condition than just
`feature = "hlsl-out"`, introduced by #5919.

Fixes #6063.
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.

2 participants