Bug 199028 – [WHLSL] Make whlsl-test-harness actually generate WHLSL shaders by default

• Component: WebGPU
• Status: RESOLVED FIXED

[Justin Fan]

The incoming whlsl test harness for layout tests should use WHLSL instead of MSL by default.

MSL support will remain as long as WebKit supports MSL for testing purposes.

[Justin Fan]

Created attachment 372666 [details]
Patch

[EWS Watchlist]

Comment on attachment 372666 [details]
Patch

Attachment 372666 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12546333

New failing tests:
http/tests/cache/disk-cache/redirect-chain-limits.html

[EWS Watchlist]

Created attachment 372670 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Justin Fan]

Test failure is a crash unrelated to this patch.

[Saam Barati]

Comment on attachment 372666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372666&action=review

> LayoutTests/webgpu/whlsl-test-harness-test.html:84
> +// This test requires a whole lot of missing operator+ and operator&& so skip for now.

is there a bug to re-enable? Can you link here as a FIXME?

> LayoutTests/webgpu/whlsl-test-harness-test.html:185
> +    // Bool functions don't compile due to bug https://bugs.webkit.org/show_bug.cgi?id=199093, so no-op for now.
> +    return;

Why make tests that depend on this then? How do you know they work? Can you write FIXME here?

> LayoutTests/webgpu/whlsl-test-harness-test.html:194
> +    for (let i = 0; i < argValues.length; ++i) {
> +        const isBuffer = Array.isArray(argValues[i]);
> +        inputArgs.push(`${isBuffer ? "device " : ""}bool${isBuffer ? "[]" : ""} in${i}`);
> +        values.push(makeBool(argValues[i]));
> +    }

You have code like this that's almost equal elsewhere but with different types. Can we just write an abstraction?

Maybe this should be in the harness itself as that abstraction?

> LayoutTests/webgpu/whlsl-test-harness-test.html:209
> +const checkFloat4s = (body, argValues = [], expected = [0, 1, 2, 3]) => {

Why isn't this in the test harness?

[Justin Fan]

(In reply to Saam Barati from comment #5)
> Comment on attachment 372666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372666&action=review
> 
> > LayoutTests/webgpu/whlsl-test-harness-test.html:84
> > +// This test requires a whole lot of missing operator+ and operator&& so skip for now.
> 
> is there a bug to re-enable? Can you link here as a FIXME?

I'll make one!

> > LayoutTests/webgpu/whlsl-test-harness-test.html:185
> > +    // Bool functions don't compile due to bug https://bugs.webkit.org/show_bug.cgi?id=199093, so no-op for now.
> > +    return;
> 
> Why make tests that depend on this then? How do you know they work? Can you
> write FIXME here?

I can write a FIXME to re-enable these; we don't know they work, but it's an example of a bug that we can use this harness to write tests for.

> > LayoutTests/webgpu/whlsl-test-harness-test.html:194
> > +    for (let i = 0; i < argValues.length; ++i) {
> > +        const isBuffer = Array.isArray(argValues[i]);
> > +        inputArgs.push(`${isBuffer ? "device " : ""}bool${isBuffer ? "[]" : ""} in${i}`);
> > +        values.push(makeBool(argValues[i]));
> > +    }
> 
> You have code like this that's almost equal elsewhere but with different
> types. Can we just write an abstraction?

Yep; I can fix up that part. I'd originally separated bool from the integral types because they need a different test function (because e.g. operator+ isn't supported for bool types).

> Maybe this should be in the harness itself as that abstraction?

That might be a good idea; I'll play with the helper functions a bit more.

> > LayoutTests/webgpu/whlsl-test-harness-test.html:209
> > +const checkFloat4s = (body, argValues = [], expected = [0, 1, 2, 3]) => {
> 
> Why isn't this in the test harness?

They aren't part of the harness because they use WPT assertions. The harness is designed to be used in conjunction with the WPT functions, but not be dependent on them. Also, they only take arguments of the listed type, but the harness functions can take arguments of any of the supported types, as long as the passed-in shader function returns the type in the name of the function.

I meant for functions like "callBoolFunction" to be the entry points to the harness. They return Promises that resolves to the results, and then we can do whatever we want with the results, i.e. assert() on them with WPT's harness.

[Saam Barati]

Comment on attachment 372666 [details]
Patch

r=me

[Justin Fan]

Created attachment 372884 [details]
Patch for landing

[Justin Fan]

Created attachment 372885 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372885 [details]
Patch for landing

Clearing flags on attachment: 372885

Committed r246824: <https://trac.webkit.org/changeset/246824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52218035>

[Radar WebKit Bug Importer]

<rdar://problem/52218036>