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 Validation tests for textureSampleCompare #3500

Merged
merged 7 commits into from
Mar 14, 2024

Conversation

greggman
Copy link
Contributor

I notice the offset option was missing in one case in the textureSample tests so that's included here. I can put it in another PR if it's important.


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.

@greggman greggman requested a review from ben-clayton March 14, 2024 01:17
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.

Looks good. Just some nits.

.fn(t => {
const { textureType, arrayIndexType, value } = t.params;
const arrayIndexArgType = kValuesTypes[arrayIndexType];
const args = [arrayIndexArgType.create(value)];
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to figure out why this was not causing failures for u32. value includes -9, -8, and I was expecting this to result in -9u, -8u which would result in: error: no matching overload for operator - (u32).

However, ScalarType.create() calls u32(value), which is implemented as:

/** Create a u32 from a numeric value, a JS `number`. */
export function u32(value: number) {
workingDataU32[0] = value;
return new U32Value(workingDataU32[0]);
}

So, we end up with the two's complement. This is unexpected in this context.

We're not trying to test for unsigned integers handling negative numbers, so I'd suggest changing the value range to just include positive numbers.

Same thing applies for the other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you're saying about -8u getting converted via Uint32Array but I need negative numbers to test offset which is required to generate an error for values < -8 and it will end up doing the same thing, passing -8 to offsetArgType.create() with those values when offsetArgType is u32.

I switched to filtering out negative values with unsigned types

@group(0) @binding(2) var<uniform> u: ${offsetArgType};
@fragment fn fs(@builtin(position) p: vec4f) -> @location(0) vec4f {
const c = 1;
let l = ${offsetArgType}(${castWGSL}(p.x));
Copy link
Contributor

Choose a reason for hiding this comment

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

The same test could be achieved with var l : ${offsetArgType};, which would reduce the complexity of the shader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needed to be var l : ${offsetArgType.create(0).wgsl()};, AFAICT. Just checking I'm not missing something.

@greggman greggman force-pushed the textureSampleCompare branch from d5d4a60 to a5d91d5 Compare March 14, 2024 17:59
@greggman greggman merged commit 7453644 into gpuweb:main Mar 14, 2024
1 check passed
@greggman greggman deleted the textureSampleCompare branch March 14, 2024 18:22
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