-
Notifications
You must be signed in to change notification settings - Fork 195
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
[wgsl-in] Implement complete validation for size and align attributes #1979
Conversation
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.
This all looks fantastic. Much cleaner. Just a few points to address.
alignment: Alignment::new(width as u32) | ||
.ok_or(LayoutErrorInner::ZeroWidth.with(ty_handle))?, |
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.
It seems to me that we need to keep this an error. Naga should never panic on bad input, and this function is used by front ends, so we can't assume the types have been validated yet.
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.
I was planning to change the width
in the IR from a u8
to either a PowerOfTwoU8
(concrete type or create some generic POT type used by width
s and the Alignment
type) or at least a NonZeroU8
. Thoughts on this?
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.
Hmm... In src/lib.rs
, at least, Bytes
is used exclusively for 2, 4, and 8. Maybe an enum like VectorSize
would suffice, with implementations of Mul
with Alignment
. Stuff like this would need to be changed to use a more relaxed type:
src/back/glsl/mod.rs
/// A scalar with an unsupported width was requested.
#[error("A scalar with an unsupported width was requested: {0:?} {1:?}")]
UnsupportedScalar(crate::ScalarKind, crate::Bytes),
But that seems like an independent change. I think that would be best as a separate PR.
@jimblandy I'm a bit confused, I thought we'll revert the removal of the error. The commits have been force-pushed but nothing seems to have changed; just a rebase? |
From the WGSL spec:
align
size
Note