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.

Sign up for GitHub

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

Jump to bottom

diag(naga): clarify ImageStore type mismatch cases #6791

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

diag(naga): clarify ImageStore type mismatch cases #6791

wants to merge 1 commit into from

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Dec 19, 2024 •
edited
Loading

Resolves #6783 (I hope 🙏🏻).

  • File follow-up to figure out why we don't have a direct argument span here.

Makes errors from source like this:

@group(0) @binding(0)
var input_texture: texture_depth_2d;
@group(0) @binding(1)
var input_sampler: sampler;
@group(0) @binding(2)
var output_texture: texture_storage_2d<r32float,write>;

@compute @workgroup_size(1, 1)
fn main() {
    let d: vec4<f32> = textureGather(input_texture, input_sampler, vec2f(0.0));
    let min_d = min(min(d[0], d[1]), min(d[2], d[3]));
    textureStore(output_texture, vec2u(1), min_d);
}

…which previously emitted an error like this:

error: Entry point main at Compute is invalid
   ┌─ in.wgsl:11:17
   │
11 │     let min_d = min(min(d[0], d[1]), min(d[2], d[3]));
   │                 ^^^ naga::Expression [11]
   │
   = The value [11] can not be stored

…to instead emit an error like this:

error: Entry point main at Compute is invalid
   ┌─ in.wgsl:11:17
   │
11 │     let min_d = min(min(d[0], d[1]), min(d[2], d[3]));
   │                 ^^^ this value is of type Scalar(Scalar { kind: Float, width: 4 })
12 │     textureStore(output_texture, vec2u(1), min_d);
   │     ^^^^^^^^^^^^ expects a value argument of type Vector { size: Quad, scalar: Scalar { kind: Float, width: 4 } }
   │
   = Image store value parameter type mismatch

@ErichDonGubler ErichDonGubler added area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator area: naga processing Passes over IR in the middle kind: diagnostics Error message should be better labels Dec 19, 2024
@ErichDonGubler ErichDonGubler self-assigned this Dec 19, 2024
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner December 19, 2024 19:41
@ErichDonGubler
Copy link
Member Author

It's not perfect, but I think it adds enough context to help a user better. I'm not sure why we don't have a direct span to the argument being provided in a call. If merged and the original issue is considered resolved, I'll file follow-up work to figure this out.

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Dec 19, 2024 •
edited
Loading

It also kills me a bit that there weren't existing tests for this; I'd really have liked for it to be easy to just fit another test case in. I think it'd be nice to have a lot of tests like this as snapshots, a la trybuild (which I know @cwfitzgerald has thought about) or our existing snapshot tests for Naga output.

@cwfitzgerald
Copy link
Member

Does naga/test/{validation.rs,wgsl_error.rs} not work for this?

@ErichDonGubler
Copy link
Member Author

Does naga/test/{validation.rs,wgsl_error.rs} not work for this?

I can add snapshot tests. I was irritated with our test suite (trybuild plz 😩), and didn't add it, but I definitely needed to be called out on adding it to this PR.

Will add it when I'm next working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teoxoy teoxoy teoxoy approved these changes

Assignees

@ErichDonGubler ErichDonGubler

Labels
area: naga processing Passes over IR in the middle area: validation Issues related to validation, diagnostics, and error handling kind: diagnostics Error message should be better naga Shader Translator
Projects
WebGPU for Firefox
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Naga error points to strange position in code for textureStore error messsage
3 participants
@ErichDonGubler @cwfitzgerald @teoxoy