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

webgpu/api/validation: Fix vertex_shader_input_location_limit test #767

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

Conversation

ben-clayton
Copy link
Contributor

The WGSL spec states that the [[location(n)]] decoration must be a "non-negative i32 literal".

const success = {
// [[location(n)]] decoration must be a "non-negative i32 literal"
// TODO: Dawn raises a "Attribute location (N) over limits" error at
// shader creation time for N>15. Is this right?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kangz - It seems to me that Dawn might be validating too early here. Please can you advise?

Copy link
Collaborator

@kainino0x kainino0x Sep 30, 2021

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

... restricted the locations themselves to be under the limit after some discussion that that was most likely sufficient, and forward-compatible with relaxing the restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Made both the shader and pipeline conditions both dependent on the location being [0..kMaxVertexAttributes)

@github-actions
Copy link

Previews, as seen when this build job started (cd2eac6):
Run tests | View tsdoc

Shader compilation / validation will fail for vertex attributes that are
out of bounds.
@ben-clayton ben-clayton marked this pull request as ready for review October 1, 2021 10:12
@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Previews, as seen when this build job started (9317e7c):
Run tests | View tsdoc

Comment on lines +97 to +105
let vsModule: GPUShaderModule | undefined;

this.expectValidationError(() => {
vsModule = this.device.createShaderModule({ code: vertexShader });
}, !success.shader);

if (vsModule === undefined) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Allowing vsModule to be undefined isn't actually necessary here (checked locally). It's technically still unsound (as is an as cast) because I don't think the compiler knows the callback of expectValidationError is always called, or that it didn't throw a caught exception. But it works anyway.

Suggested change
let vsModule: GPUShaderModule | undefined;
this.expectValidationError(() => {
vsModule = this.device.createShaderModule({ code: vertexShader });
}, !success.shader);
if (vsModule === undefined) {
return;
}
let vsModule: GPUShaderModule;
this.expectValidationError(() => {
vsModule = this.device.createShaderModule({ code: vertexShader });
}, !success.shader);

this.expectValidationError(() => {
this.device.createRenderPipeline({
vertex: {
module: vsModule,
module: vsModule as GPUShaderModule,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module: vsModule as GPUShaderModule,
module: vsModule,

const success = testLocation < kMaxVertexAttributes;
const success = {
shader: testLocation >= 0 && testLocation < kMaxVertexAttributes,
pipeline: testLocation >= 0 && testLocation < kMaxVertexAttributes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case the pipeline creation would actually fail for either of two separate reasons: shader module was invalid, and shaderLocation was out of bounds. We can't tell which one.

If it's allowed for a shader to have a subset of the attributes defined by the pipeline (I'm not sure, but I think it is allowed), only the shaderLocation being out of bounds should be tested here.

@@ -396,7 +413,10 @@ g.test('vertex_shader_input_location_limit')
},
];

const success = testLocation < kMaxVertexAttributes;
const success = {
shader: testLocation >= 0 && testLocation < kMaxVertexAttributes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that this is only used in one test kind of indicates this test should be somewhere else - it's a test of createShaderModule and not of the vertex state setup in createRenderPipeline.

If indeed this should fail in createShaderModule, then I think it's really a shader validation test (src/webgpu/shader/validation/)

@dj2 dj2 added the wgsl label Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants