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
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 40 additions & 19 deletions src/webgpu/api/validation/vertex_state.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,22 +83,31 @@ class F extends ValidationTest {
}

testVertexState(
success: boolean,
success: { shader: boolean; pipeline: boolean },
buffers: Iterable<GPUVertexBufferLayout>,
vertexShader: string = VERTEX_SHADER_CODE_WITH_NO_INPUT
) {
const vsModule = this.device.createShaderModule({ code: vertexShader });
const fsModule = this.device.createShaderModule({
code: `
[[stage(fragment)]] fn main() -> [[location(0)]] vec4<f32> {
return vec4<f32>(0.0, 1.0, 0.0, 1.0);
}`,
});

let vsModule: GPUShaderModule | undefined;

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

if (vsModule === undefined) {
return;
}
Comment on lines +97 to +105
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,

entryPoint: 'main',
buffers,
},
Expand All @@ -109,7 +118,7 @@ class F extends ValidationTest {
},
primitive: { topology: 'triangle-list' },
});
}, !success);
}, !success.pipeline);
}

generateTestVertexShader(inputs: { type: string; location: number }[]): string {
Expand Down Expand Up @@ -164,7 +173,7 @@ g.test('max_vertex_buffer_limit')
}
}

const success = count <= kMaxVertexBuffers;
const success = { shader: true, pipeline: count <= kMaxVertexBuffers };
t.testVertexState(success, vertexBuffers);
});

Expand Down Expand Up @@ -201,7 +210,7 @@ g.test('max_vertex_attribute_limit')
vertexBuffers.push({ arrayStride: 0, attributes });
}

const success = attribCount <= kMaxVertexAttributes;
const success = { shader: true, pipeline: attribCount <= kMaxVertexAttributes };
t.testVertexState(success, vertexBuffers);
});

Expand Down Expand Up @@ -229,7 +238,7 @@ g.test('max_vertex_buffer_array_stride_limit')
const vertexBuffers = [];
vertexBuffers[vertexBufferIndex] = { arrayStride, attributes: [] };

const success = arrayStride <= kMaxVertexBufferArrayStride;
const success = { shader: true, pipeline: arrayStride <= kMaxVertexBufferArrayStride };
t.testVertexState(success, vertexBuffers);
});

Expand Down Expand Up @@ -258,7 +267,7 @@ g.test('vertex_buffer_array_stride_limit_alignment')
const vertexBuffers = [];
vertexBuffers[vertexBufferIndex] = { arrayStride, attributes: [] };

const success = arrayStride % 4 === 0;
const success = { shader: true, pipeline: arrayStride % 4 === 0 };
t.testVertexState(success, vertexBuffers);
});

Expand Down Expand Up @@ -295,7 +304,7 @@ g.test('vertex_attribute_shaderLocation_limit')
const vertexBuffers = [];
vertexBuffers[vertexBufferIndex] = { arrayStride: 256, attributes };

const success = testShaderLocation < kMaxVertexAttributes;
const success = { shader: true, pipeline: testShaderLocation < kMaxVertexAttributes };
t.testVertexState(success, vertexBuffers);
});

Expand Down Expand Up @@ -360,18 +369,26 @@ g.test('vertex_attribute_shaderLocation_unique')

// Note that an empty vertex shader will be used so errors only happens because of the conflict
// in the vertex state.
const success = shaderLocationA !== shaderLocationB;
const success = { shader: true, pipeline: shaderLocationA !== shaderLocationB };
t.testVertexState(success, vertexBuffers);
});

g.test('vertex_shader_input_location_limit')
.desc(
`Test that vertex shader's input's location decoration must be less than maxVertexAttributes.
- Test for shaderLocation 0, 1, limit - 1, limit`
- Test for shaderLocation 0, 1, limit - 1, limit, -1, 0x7fffffff, 2 ** 32`
)
.paramsSubcasesOnly(u =>
u //
.combine('testLocation', [0, 1, kMaxVertexAttributes - 1, kMaxVertexAttributes, -1, 2 ** 32])
.combine('testLocation', [
0,
1,
kMaxVertexAttributes - 1,
kMaxVertexAttributes,
-1,
0x7fffffff,
2 ** 32,
])
)
.fn(t => {
const { testLocation } = t.params;
Expand All @@ -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/)

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.

};
t.testVertexState(success, vertexBuffers, shader);
});

Expand Down Expand Up @@ -438,14 +458,15 @@ g.test('vertex_shader_input_location_in_vertex_state')
extraAttributeCount,
extraAttributeSkippedLocations: [testShaderLocation],
});
t.testVertexState(false, vertexBuffers, shader);

t.testVertexState({ shader: true, pipeline: false }, vertexBuffers, shader);

// Add an attribute for the test location and try again.
addTestAttributes(attributes, {
testAttribute: { format: 'float32', shaderLocation: testShaderLocation, offset: 0 },
testAttributeAtStart,
});
t.testVertexState(true, vertexBuffers, shader);
t.testVertexState({ shader: true, pipeline: true }, vertexBuffers, shader);
});

g.test('vertex_shader_type_matches_attribute_format')
Expand Down Expand Up @@ -484,7 +505,7 @@ g.test('vertex_shader_type_matches_attribute_format')
float: 'f32',
}[kVertexFormatInfo[format].type];

const success = requiredBaseType === shaderBaseType;
const success = { shader: true, pipeline: requiredBaseType === shaderBaseType };
t.testVertexState(
success,
[
Expand Down Expand Up @@ -554,7 +575,7 @@ g.test('vertex_attribute_offset_alignment')

const formatInfo = kVertexFormatInfo[format];
const formatSize = formatInfo.bytesPerComponent * formatInfo.componentCount;
const success = offset % Math.min(4, formatSize) === 0;
const success = { shader: true, pipeline: offset % Math.min(4, formatSize) === 0 };

t.testVertexState(success, vertexBuffers);
});
Expand Down Expand Up @@ -628,7 +649,7 @@ g.test('vertex_attribute_contained_in_stride')
const formatSize = formatInfo.bytesPerComponent * formatInfo.componentCount;
const limit = arrayStride === 0 ? kMaxVertexBufferArrayStride : arrayStride;

const success = offset + formatSize <= limit;
const success = { shader: true, pipeline: offset + formatSize <= limit };
t.testVertexState(success, vertexBuffers);
});

Expand All @@ -642,5 +663,5 @@ g.test('many_attributes_overlapping')
attributes.push({ format: formats[i % 3], offset: i * 4, shaderLocation: i } as const);
}

t.testVertexState(true, [{ arrayStride: 0, attributes }]);
t.testVertexState({ shader: true, pipeline: true }, [{ arrayStride: 0, attributes }]);
});