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

Make spirv an optional feature #1518

Merged
merged 2 commits into from
Jun 22, 2021
Merged

Make spirv an optional feature #1518

merged 2 commits into from
Jun 22, 2021

Conversation

kvark
Copy link
Member

@kvark kvark commented Jun 19, 2021

Connections
gfx-rs/naga#940 shows how much SPIR-V parsing can be a pain.

Description
Keep it supported natively, but put it behind a feature flag. This allows to skip compilation of parts of Naga as well as dependencies like petgraph.
On my machine, compiling wgpu-core time is reduced from 40.87s to 35.36s, which is about 13% improvement.

Testing
Just compiling

@ElectronicRU
Copy link
Contributor

Regarding #1520 -- should that still be enabled without spirv? I'd say yes because this feature is related to parsing priv, not using spirv, but the name could get confusing.

@kvark
Copy link
Member Author

kvark commented Jun 20, 2021

@ElectronicRU Yes, I was just thinking about the same. Perhaps, we should adjust the (run-time) feature name to spell it more clearly.
Something like: SPIRV_SHADER_PASSTHROUGH? If it's just SPIRV_SHADERS then it's easy to confuse with the effect of the compile-time feature in this PR.

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.

I think we should go ahead with this, the compile time improvement is nothing to shake a stick at

@grovesNL
Copy link
Collaborator

Interestingly if we did allow naga modules to be provided as mentioned on Matrix recently, we wouldn't need SPIR-V specific options here. i.e. the application/library could opt-in to the SPIR-V frontend in naga

@cwfitzgerald
Copy link
Member

It does predicate that naga can actually parse the spirv, which is one of the things passing raw spirv is trying to get around in #1525

@kvark
Copy link
Member Author

kvark commented Jun 21, 2021

@grovesNL indeed. It would have to be passed by move though, because wgpu wants to own Naga's module, but these modules aren't cloneable (at least today).

@kvark
Copy link
Member Author

kvark commented Jun 21, 2021

@grovesNL @cwfitzgerald I added a commit that removes SPIR-V support entirely from wgc. The support on wgpu-rs side is unchanged.

One of the nasty trade-offs here is being able to API trace the run involving SPIR-V. Since it's not in wgc, we have to dump the Naga IR, which requires "serialize" feature to be enabled. I thought that would increase the build times, but I wasn't able to see any difference locally. It will not affect most users, supposedly, because trace isn't enabled by default.

@cwfitzgerald
Copy link
Member

I'd be really hesitant to merge removing spirv entirely right now. I do think this is eventually where we want to be, but having things be predicated on naga understanding every shader means that a good chunk of our features are unusable because naga is yet unaware of them. spirv was basically our last way around any current limitations of naga. I think in some time we should reconsider.

@kvark
Copy link
Member Author

kvark commented Jun 22, 2021

spirv was basically our last way around any current limitations of naga. I think in some time we should reconsider.

This functionality is here, it stays, and it's no directly related to this PR. See #1525

Also, the current state of the PR is still keeping SPIRV parsing support, just at wgpu-rs level instead of wgpu-core level.

@cwfitzgerald
Copy link
Member

Oh I see, so we're just removing spirv ingestion from wgpu-core in the safe path. That sounds reasonable to me. I've got some nits, but otherwise LGTM.

wgpu/README.md Outdated Show resolved Hide resolved
wgpu-core/src/device/mod.rs Show resolved Hide resolved
@kvark
Copy link
Member Author

kvark commented Jun 22, 2021

bors r=cwfitzgerald

@bors
Copy link
Contributor

bors bot commented Jun 22, 2021

@bors bors bot merged commit 5f2df9a into gfx-rs:master Jun 22, 2021
@kvark kvark deleted the spirv branch June 22, 2021 16:55
Patryk27 pushed a commit to Patryk27/wgpu that referenced this pull request Nov 23, 2022
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.

5 participants