-
Notifications
You must be signed in to change notification settings - Fork 905
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
feature: Parse enable directives & SHADER-F16 support #5701
base: trunk
Are you sure you want to change the base?
Conversation
I've marked this as ready for review, as the wgpu specific logic is implemented and would be great to start iterating on it. 2 main blockers are:
|
Dogfooding works! Fixed a few bugs in 30e12b5. Still waiting on upstream. |
use std::hash::Hash; | ||
|
||
#[derive(Debug, Default)] | ||
pub struct TranslationUnit<'a> { | ||
pub directives: Arena<GlobalDirective>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this field at all, we might in the future but I think it will end up looking differently. Extensions and language extensions can be bitflags, and global diagnostic modifiers will have their own shapes.
I think we should either remove this field or replace it with an extensions bitflags.
Could you squash all the existing commits into 1? It will make it easier to look at the PR once the feedback is addressed. |
@teoxoy 100% - thanks for the review let me address and squash! |
One thing I'm concerned is that the polyfills we have in some of the backends might not support |
It would be best for review to squash first so that the commits addressing various parts are by themselves. |
9c6a9b3
to
9f33e0e
Compare
I have not reviewed this PR in-depth as @teoxoy has, but the broad strokes of the const. eval code look correct to me, FWIW. 🫡 |
7906c11
to
97589b3
Compare
self.capabilities_used | ||
.insert(spirv::Capability::StorageBuffer16BitAccess); | ||
self.capabilities_used | ||
.insert(spirv::Capability::UniformAndStorageBuffer16BitAccess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoxoy not actually sure about this now.
gpuweb/gpuweb#2512 (comment)
Seems some of these are optional. If you enable them all you lose most devices on Vulkan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What ended up in the spec requires all of the Vulkan features. Some more context from the follow-up meetings:
gpuweb/gpuweb#2512 (comment)
gpuweb/gpuweb#2512 (comment)
@FL33TW00D please request a re-review once ready. |
@teoxoy will do apologies this got deprioritised. |
No worries, I just want to make sure to get back to it. |
Would it be feasible, either here or in a followup, to support the builtin Bytemuck just needs its |
The buffers are all just a bunch of bytes, so you can use whichever type you want then put it in the buffer. The usage of |
Taking a look at this again @teoxoy on a rented EC2 machine to check Vulkan.
This is because the Any ideas here? I'd expect a T4 to offer this? EDIT:
|
from gpuweb/gpuweb#2512 I think we have to do the casting ourselves. Dawn also seems to do this. We can do it in a different PR though. What I find interesting is that the proposal talked about SM6.2 & Native16BitShaderOpsSupported enabling support for f16 IO, I guess the Nvidia compiler that ingests DXIL will do the casting. It probably does the casting regardless of frontend and that's why people report this working on Vulkan as well despite |
## To Run | ||
|
||
``` | ||
RUST_LOG=hello_compute cargo run --bin wgpu-examples shader_f16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why is RUST_LOG
set to hello_compute
, which is the name of a different example? Is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste error!
eebedb1
to
f4e1d2e
Compare
f4e1d2e
to
b936c71
Compare
I've taken the liberty of rebasing this PR, applying the following notable changes:
There are still numerous CI failures, but I believe that my rebasing onto |
Resolves #5476.
Resolves #4384.
References:
F16 is available in >=SM6.2: https://github.com/microsoft/DirectXShaderCompiler/wiki/16-Bit-Scalar-Types