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

Add missing tests for Adapter Capability Guarantees #3107

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

beaufortfrancois
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Oct 27, 2023

Issue: #3106

This PR adds missing tests for https://gpuweb.github.io/gpuweb/#adapter-capability-guarantees


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

It works in Chromium with the https://chromium-review.googlesource.com/c/chromium/src/+/4952541 CL.

image

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

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

@beaufortfrancois
Copy link
Collaborator Author

https://gpuweb.github.io/cts/standalone/?q=webgpu:api,validation,capability_checks,limits seems to about validation, while this is specifically about operation. It doesn't require any pipeline, only requestAdapter().

Isn't all of this already tested here?

Sadly no, here's an exhaustive list of things that are not tested:

  • At least one of the following must be true:
    • "texture-compression-bc" is supported.
    • Both "texture-compression-etc2" and "texture-compression-astc" are supported.
  • maxBindGroups must be ≤ maxBindGroupsPlusVertexBuffers.
  • maxVertexBuffers must be ≤ maxBindGroupsPlusVertexBuffers.
  • maxStorageBufferBindingSize must be a multiple of 4 bytes.
  • maxVertexBufferArrayStride must be a multiple of 4 bytes.
  • maxComputeWorkgroupSizeX must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeWorkgroupSizeY must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeWorkgroupSizeZ must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeInvocationsPerWorkgroup must be ≤ maxComputeWorkgroupSizeX × maxComputeWorkgroupSizeY × maxComputeWorkgroupSizeZ.

I've noticed some of them that were tested already indeed though. Thanks for catching! See:

  • maxBindingsPerBindGroup must be must be ≥ (max bindings per shader stage × max shader stages per pipeline)
  • All alignment-class limits must be powers of 2.
  • minUniformBufferOffsetAlignment and minStorageBufferOffsetAlignment must both be ≥ 32 bytes.
  • maxUniformBufferBindingSize must be ≤ maxBufferSize.
  • maxStorageBufferBindingSize must be ≤ maxBufferSize.

I'd be in favor of moving the existing tests in operation as they're not about validation and reflect adapter, not device.
What do you think?

@greggman
Copy link
Contributor

  • maxBindGroups must be ≤ maxBindGroupsPlusVertexBuffers.
  • maxVertexBuffers must be ≤ maxBindGroupsPlusVertexBuffers.
  • maxStorageBufferBindingSize must be a multiple of 4 bytes.
  • maxVertexBufferArrayStride must be a multiple of 4 bytes.
  • maxComputeWorkgroupSizeX must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeWorkgroupSizeY must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeWorkgroupSizeZ must be ≤ maxComputeInvocationsPerWorkgroup.
  • maxComputeInvocationsPerWorkgroup must be ≤ maxComputeWorkgroupSizeX × maxComputeWorkgroupSizeY × maxComputeWorkgroupSizeZ.

To stay consistent these seem like they should all be checked in their respective limits test.

See:

g.test('validate,maxColorAttachmentBytesPerSample')

as examples of similar things being tested.

As for whether they belong in operations. There is no operation, they're is just validating that the limits are what they're supposed to be and that when you ask for higher ones you get higher ones. Kai told me to put them in validation so I did but I have no objection to moving them personally.

@beaufortfrancois
Copy link
Collaborator Author

I've updated this PR with your suggested changes. Please have a look @greggman .

@beaufortfrancois beaufortfrancois changed the title Add tests for Adapter Capability Guarantees Add missing tests for Adapter Capability Guarantees Oct 30, 2023
@github-actions
Copy link

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

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

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

lgtm

@beaufortfrancois
Copy link
Collaborator Author

Thank you @greggman for reviewing!

@beaufortfrancois beaufortfrancois merged commit 3dbe4ce into main Oct 30, 2023
@beaufortfrancois beaufortfrancois deleted the adapter-capability branch October 30, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants