-
Notifications
You must be signed in to change notification settings - Fork 103
Add immediate size validation cases for pipeline creation #4573
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,8 @@ Note: entry point matching tests are in shader_module/entry_point.spec.ts | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { AllFeaturesMaxLimitsGPUTest } from '../.././gpu_test.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { makeTestGroup } from '../../../common/framework/test_group.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { keysOf } from '../../../common/util/data_tables.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getGPU } from '../../../common/util/navigator_gpu.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { assert, supportsImmediateData } from '../../../common/util/util.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isTextureFormatUsableWithStorageAccessMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kPossibleStorageTextureFormats, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -811,3 +813,94 @@ generates a validation error at createComputePipeline(Async) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vtu.doCreateComputePipelineTest(t, isAsync, success, descriptor); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| g.test('pipeline_creation_immediate_size_mismatch') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .desc( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Validate that creating a pipeline fails if the shader uses immediate data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| larger than the immediateSize specified in the pipeline layout, or larger than | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maxImmediateSize if layout is 'auto'. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Also validates that using less or equal size is allowed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .params(u => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const kNumericCases = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 16, layoutSize: 16 }, // Equal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 12, layoutSize: 16 }, // Shader smaller | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 20, layoutSize: 16 }, // Shader larger (small diff) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 32, layoutSize: 16 }, // Shader larger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const kMaxLimitsCases = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 'max', layoutSize: 'auto' }, // Shader equal to limit (auto layout) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { shaderSize: 'exceedLimits', layoutSize: 'auto' }, // Shader larger than limit (auto layout) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return u | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .combine('isAsync', [true, false]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .combineWithParams([...kNumericCases, ...kMaxLimitsCases] as const); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+826
to
+840
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This pattern is interesting, but I think I find it slightly harder to read than the more typical pattern that makes it clearer that these two arrays will be part of the same Though the header comments aren't really necessary because it's already obvious what it means.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Learned this shape from copilot comments :) Glad to back to the common pattern. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .fn(t => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.skipIf(!supportsImmediateData(getGPU(t.rec)), 'Immediate data not supported'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { isAsync, shaderSize, layoutSize } = t.params; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert(t.device.limits.maxImmediateSize !== undefined); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const maxImmediateSize = t.device.limits.maxImmediateSize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+846
to
+847
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
shaoboyan091 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let actualLayout: GPUPipelineLayout | 'auto'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let validSize: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (layoutSize === 'auto') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualLayout = 'auto'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validSize = maxImmediateSize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualLayout = t.device.createPipelineLayout({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bindGroupLayouts: [], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| immediateSize: layoutSize as number, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validSize = layoutSize as number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let actualShaderSize: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (shaderSize === 'max') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualShaderSize = validSize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (shaderSize === 'exceedLimits') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualShaderSize = validSize + 4; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualShaderSize = shaderSize as number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think the type narrowing makes this cast unnecessary
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure the test's fixed sizes fit within the device limit. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (shaderSize !== 'exceedLimits') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actualShaderSize <= maxImmediateSize, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `shaderSize (${actualShaderSize}) must be <= maxImmediateSize (${maxImmediateSize})` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const numFields = actualShaderSize / 4; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const fields = Array.from({ length: numFields }, (_, i) => `m${i}: u32`).join(', '); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const code = ` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct Immediates { ${fields} } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var<immediate> data: Immediates; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn use_data() { _ = data.m0; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @compute @workgroup_size(1) fn main_compute() { use_data(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const shouldError = actualShaderSize > validSize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // When the shader exceeds the device's maxImmediateSize, the error occurs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // at shader module creation time, not pipeline creation time. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+891
to
+892
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be validated here, if we can avoid it. createShaderModule doesn't know whether a given entry point is going to be used or not. I am pretty sure WebGPU doesn't normally validate features or limits in createShaderModule. Also, if we do end up allowing arrays, then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, makes sense. Then I think that's another implementation bug. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (layoutSize === 'auto' && shouldError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.expectValidationError(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.device.createShaderModule({ code }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| vtu.doCreateComputePipelineTest(t, isAsync, !shouldError, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| layout: actualLayout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| compute: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| module: t.device.createShaderModule({ code }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ misc createRenderPipeline and createRenderPipelineAsync validation tests. | |
| `; | ||
|
|
||
| import { makeTestGroup } from '../../../../common/framework/test_group.js'; | ||
| import { getGPU } from '../../../../common/util/navigator_gpu.js'; | ||
| import { assert, supportsImmediateData } from '../../../../common/util/util.js'; | ||
| import { | ||
| isTextureFormatUsableWithStorageAccessMode, | ||
| kPossibleStorageTextureFormats, | ||
|
|
@@ -119,8 +121,10 @@ g.test('pipeline_layout,device_mismatch') | |
| }); | ||
|
|
||
| g.test('external_texture') | ||
| .desc('Tests createRenderPipeline() with an external_texture') | ||
| .desc('Tests createRenderPipeline(Async) with an external_texture') | ||
| .params(u => u.combine('isAsync', [false, true])) | ||
| .fn(t => { | ||
| const { isAsync } = t.params; | ||
| const shader = t.device.createShaderModule({ | ||
| code: ` | ||
| @vertex | ||
|
|
@@ -149,7 +153,7 @@ g.test('external_texture') | |
| }, | ||
| }; | ||
|
|
||
| vtu.doCreateRenderPipelineTest(t, false, true, descriptor); | ||
| vtu.doCreateRenderPipelineTest(t, isAsync, true, descriptor); | ||
| }); | ||
|
|
||
| g.test('storage_texture,format') | ||
|
|
@@ -192,3 +196,136 @@ generates a validation error at createComputePipeline(Async) | |
| }; | ||
| vtu.doCreateRenderPipelineTest(t, isAsync, success, descriptor); | ||
| }); | ||
|
|
||
| g.test('pipeline_creation_immediate_size_mismatch') | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The render and compute tests are extremely similar, I would prefer to put them together in the same file (and probably share some code), e.g. Not sure how easy this is to do - consider it, but it's not that important. |
||
| .desc( | ||
| ` | ||
| Validate that creating a pipeline fails if the shader uses immediate data | ||
| larger than the immediateSize specified in the pipeline layout, or larger than | ||
| maxImmediateSize if layout is 'auto'. | ||
| Also validates that using less or equal size is allowed. | ||
| ` | ||
| ) | ||
| .params(u => { | ||
| const kNumericCases = [ | ||
| { vertexSize: 16, fragmentSize: 16, layoutSize: 16 }, // Equal | ||
| { vertexSize: 12, fragmentSize: 12, layoutSize: 16 }, // Shader smaller | ||
| { vertexSize: 20, fragmentSize: 20, layoutSize: 16 }, // Shader larger (small diff) | ||
| { vertexSize: 32, fragmentSize: 32, layoutSize: 16 }, // Shader larger | ||
| ] as const; | ||
shaoboyan091 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const kMaxLimitsCases = [ | ||
| { vertexSize: 'max', fragmentSize: 0, layoutSize: 'auto' }, // Vertex = Limit (Control) | ||
| { vertexSize: 0, fragmentSize: 'max', layoutSize: 'auto' }, // Fragment = Limit (Control) | ||
| { vertexSize: 'max', fragmentSize: 'max', layoutSize: 'auto' }, // Both at Limit (Control) | ||
| { vertexSize: 'exceedLimits', fragmentSize: 0, layoutSize: 'auto' }, // Vertex > Limit | ||
| { vertexSize: 0, fragmentSize: 'exceedLimits', layoutSize: 'auto' }, // Fragment > Limit | ||
| ] as const; | ||
| return u | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same suggestion here |
||
| .combine('isAsync', [true, false]) | ||
| .combineWithParams([...kNumericCases, ...kMaxLimitsCases] as const); | ||
| }) | ||
| .fn(t => { | ||
| t.skipIf(!supportsImmediateData(getGPU(t.rec)), 'Immediate data not supported'); | ||
|
|
||
| const { isAsync, vertexSize, fragmentSize, layoutSize } = t.params; | ||
|
|
||
| assert(t.device.limits.maxImmediateSize !== undefined); | ||
| const maxImmediateSize = t.device.limits.maxImmediateSize; | ||
|
Comment on lines
+232
to
+233
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
|
|
||
| const resolveSize = (sizeDescriptor: number | string) => { | ||
| if (typeof sizeDescriptor === 'number') return sizeDescriptor; | ||
| if (sizeDescriptor === 'max') return maxImmediateSize; | ||
| if (sizeDescriptor === 'exceedLimits') return maxImmediateSize + 4; | ||
| return 0; | ||
| }; | ||
|
|
||
| const resolvedVertexImmediateSize = resolveSize(vertexSize); | ||
| const resolvedFragmentImmediateSize = resolveSize(fragmentSize); | ||
|
|
||
| // Ensure the test's fixed sizes fit within the device limit. | ||
| if (vertexSize !== 'exceedLimits') { | ||
| assert( | ||
| resolvedVertexImmediateSize <= maxImmediateSize, | ||
| `vertexSize (${resolvedVertexImmediateSize}) must be <= maxImmediateSize (${maxImmediateSize})` | ||
| ); | ||
| } | ||
| if (fragmentSize !== 'exceedLimits') { | ||
| assert( | ||
| resolvedFragmentImmediateSize <= maxImmediateSize, | ||
| `fragmentSize (${resolvedFragmentImmediateSize}) must be <= maxImmediateSize (${maxImmediateSize})` | ||
| ); | ||
| } | ||
|
|
||
| // Helper to generate a stage-specific shader module with the given immediate data size. | ||
| const makeShaderCode = (size: number, stage: 'vertex' | 'fragment') => { | ||
| if (size === 0) { | ||
| if (stage === 'vertex') { | ||
| return `@vertex fn main_vertex() -> @builtin(position) vec4<f32> { return vec4<f32>(0.0, 0.0, 0.0, 1.0); }`; | ||
| } | ||
| return `@fragment fn main_fragment() -> @location(0) vec4<f32> { return vec4<f32>(0.0, 1.0, 0.0, 1.0); }`; | ||
| } | ||
| const numFields = size / 4; | ||
| const fields = Array.from({ length: numFields }, (_, i) => `m${i}: u32`).join(', '); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| if (stage === 'vertex') { | ||
| return ` | ||
| struct Immediates { ${fields} } | ||
| var<immediate> data: Immediates; | ||
| @vertex fn main_vertex() -> @builtin(position) vec4<f32> { _ = data.m0; return vec4<f32>(0.0, 0.0, 0.0, 1.0); } | ||
| `; | ||
| } | ||
| return ` | ||
| struct Immediates { ${fields} } | ||
| var<immediate> data: Immediates; | ||
| @fragment fn main_fragment() -> @location(0) vec4<f32> { _ = data.m0; return vec4<f32>(0.0, 1.0, 0.0, 1.0); } | ||
| `; | ||
| }; | ||
|
|
||
| let layout: GPUPipelineLayout | 'auto'; | ||
| let validSize: number; | ||
|
|
||
| if (layoutSize === 'auto') { | ||
| layout = 'auto'; | ||
| validSize = maxImmediateSize; | ||
| } else { | ||
| layout = t.device.createPipelineLayout({ | ||
| bindGroupLayouts: [], | ||
| immediateSize: layoutSize as number, | ||
| }); | ||
| validSize = layoutSize as number; | ||
| } | ||
|
|
||
| const vertexExceedsLimit = resolvedVertexImmediateSize > validSize; | ||
| const fragmentExceedsLimit = resolvedFragmentImmediateSize > validSize; | ||
| const shouldError = vertexExceedsLimit || fragmentExceedsLimit; | ||
|
|
||
| // When the shader exceeds the device's maxImmediateSize, the error occurs | ||
| // at shader module creation time, not pipeline creation time. | ||
|
Comment on lines
+301
to
+302
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| // Create each shader module separately so the correct one gets the error. | ||
| const vertexCode = makeShaderCode(resolvedVertexImmediateSize, 'vertex'); | ||
| const fragmentCode = makeShaderCode(resolvedFragmentImmediateSize, 'fragment'); | ||
|
|
||
| if (layoutSize === 'auto' && vertexExceedsLimit) { | ||
| t.expectValidationError(() => { | ||
| t.device.createShaderModule({ code: vertexCode }); | ||
| }); | ||
| } | ||
| if (layoutSize === 'auto' && fragmentExceedsLimit) { | ||
| t.expectValidationError(() => { | ||
| t.device.createShaderModule({ code: fragmentCode }); | ||
| }); | ||
| } | ||
| if (layoutSize === 'auto' && shouldError) { | ||
| return; | ||
| } | ||
|
|
||
| vtu.doCreateRenderPipelineTest(t, isAsync, !shouldError, { | ||
| layout, | ||
| vertex: { | ||
| module: t.device.createShaderModule({ code: vertexCode }), | ||
| }, | ||
| fragment: { | ||
| module: t.device.createShaderModule({ code: fragmentCode }), | ||
| targets: [{ format: 'rgba8unorm' }], | ||
| }, | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.