Skip to content

Conversation

@zoddicus
Copy link
Contributor

Fixes #3174


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.

@zoddicus
Copy link
Contributor Author

Another stacked change, all the new code is in 744e31b

Copy link
Contributor

@jiangzhaoming jiangzhaoming left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks

Copy link
Contributor

@ben-clayton ben-clayton left a comment

Choose a reason for hiding this comment

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

LGTM, however I'm not sure if the cache should be re-generated or not based on the code changes. It might be worth checking whether you get the same diffs if we all agree on a NodeJS version to use. See #3187

1,
...fullF16Range(),
];
const cases = (['f32', 'f16'] as const)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this and the other changes, a comment describing what cases holds (non-exhaustive fields) would be good as this made me go cross-eyed trying to figure out what it was actually doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think something like

// Cases: [f32|f16]_[non_]const

is sufficient?

This is what jzm has included for the usage of this pattern that they wrote, but I didn't replicate consistantly.

@zoddicus
Copy link
Contributor Author

LGTM, however I'm not sure if the cache should be re-generated or not based on the code changes. It might be worth checking whether you get the same diffs if we all agree on a NodeJS version to use. See #3187

I am currently using 20.9.0, which I believe is the newest LTS release and the same one that you pin in that PR.

This PR shouldn't be changing the actual expectations significantly (There are a couple of tests that were using the wrong generator that will be changing, but most should be a no-op). It does change the code in floating_point.ts and the various .spec.ts files, so I think they should have a different hash.

@ben-clayton
Copy link
Contributor

SGTM then, thanks!

@zoddicus zoddicus merged commit b37df1c into gpuweb:main Nov 23, 2023
@zoddicus zoddicus deleted the addMatrixRangeAPI branch November 23, 2023 18:44
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.

wgsl: Expose various test range generators in FPTraits

3 participants