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

wgsl: Add countLeadingZeros tests #1001

Merged
merged 1 commit into from Feb 24, 2022

Conversation

ben-clayton
Copy link
Contributor

Issue: #


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.)

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.

@github-actions
Copy link

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

@github-actions
Copy link

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

@ben-clayton ben-clayton marked this pull request as ready for review February 23, 2022 17:48
Copy link
Contributor

@zoddicus zoddicus left a comment

Choose a reason for hiding this comment

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

So the actual tests look good to me, there is just some process side issues.
This builtin doesn't exist for the specURL, since it is a older revision. I have commented an updated stanza that links to a version of the spec that contains the builtin.

The larger issue I noticed is that currently the spec specifies the u32 and i32 cases in the same rule, which means there is only one .uniqueID(71610deeeb6a9fd3) for both tests. Other builtins, like clamp have seperate u32 and i32 cases in the spec, thus separate uniqueIDs. I am honestly not sure how much of a mechanical problem this is, i.e. if it is going to cause some tooling to break, but I suspect this is something that someone will have to cleanup in the future if we just used it twice, since it is supposed to unique.

That being said, I can think of two immediate solutions, a high road a low road one.

High road:
Update the spec to have separate u32 and i32 rules, so there are two unique IDs. I would probably update the other rules you are writing test for also in one PR, so they all have separate IDs.

Low Road:
Merge the two test in the CTS into one uber test that does both. This strikes me a potentially fraught though, since the framework distinguishes between u32 and i32 inputs, so we would need some sort of cludge to allow both, which doesn't seem like a desired feature.

Also there isn't a tracking bug filed for this and the other tests you are adding. I needed to update the specURL in my local checkout of get-test-plan to generate the rules so I could grab the unique IDs, etc. I can file the issues if you want, once we figure out the uniqueID situation.


g.test('integer_builtin_functions,countLeadingZeros_unsigned')
.uniqueId('xxxxxxxxxxxxxxxx')
.specURL('https://www.w3.org/TR/2021/WD-WGSL-20210929/#integer-builtin-functions')
Copy link
Contributor

Choose a reason for hiding this comment

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

.specURL(https://www.w3.org/TR/2022/WD-WGSL-20220223/#integer-builtin-functions), since this doesn't exist in the older spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this matches the URL of all the other test plans, which will also need bulk updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example:

.specURL('https://www.w3.org/TR/2021/WD-WGSL-20210929/#integer-builtin-functions')

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel particularly strongly about this, since as you pointed out it matches the existing tests and likely needs a bulk update across the code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's land these then, and I'll do a bulk update of all of these in a single atomic commit.


g.test('integer_builtin_functions,countLeadingZeros_signed')
.uniqueId('xxxxxxxxxxxxxxxx')
.specURL('https://www.w3.org/TR/2021/WD-WGSL-20210929/#integer-builtin-functions')
Copy link
Contributor

Choose a reason for hiding this comment

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

@zoddicus
Copy link
Contributor

Looking at the spec again, it looks like all of the older integer builtins have distinct u32 and i32 rules, so I think the high road solution is the correct way forward.

@zoddicus
Copy link
Contributor

Talked with Ben about this out of band, and I think it is fine to leave this PR as-is other than the invalid URL, which I commented on. We will need to resolve some of the tooling and style questions in the future, but nothing automated is going to break, so it isn't worth slowing down getting tests right now.

@zoddicus zoddicus self-requested a review February 23, 2022 20:30
@ben-clayton ben-clayton merged commit e8809ad into gpuweb:main Feb 24, 2022
@ben-clayton ben-clayton deleted the countLeadingZeros branch February 24, 2022 12:35
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.

None yet

2 participants