Bug 199028 - [WHLSL] Make whlsl-test-harness actually generate WHLSL shaders by default
Summary: [WHLSL] Make whlsl-test-harness actually generate WHLSL shaders by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on: 198978
Blocks: 195681
  Show dependency treegraph
 
Reported: 2019-06-19 13:12 PDT by Justin Fan
Modified: 2019-06-26 16:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (41.83 KB, patch)
2019-06-21 19:05 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.75 MB, application/zip)
2019-06-21 21:05 PDT, Build Bot
no flags Details
Patch for landing (42.75 KB, patch)
2019-06-25 17:27 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (42.83 KB, patch)
2019-06-25 17:32 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-06-19 13:12:31 PDT
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.
Comment 1 Justin Fan 2019-06-21 19:05:34 PDT
Created attachment 372666 [details]
Patch
Comment 2 Build Bot 2019-06-21 21:05:24 PDT
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
Comment 3 Build Bot 2019-06-21 21:05:25 PDT
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
Comment 4 Justin Fan 2019-06-24 11:48:36 PDT
Test failure is a crash unrelated to this patch.
Comment 5 Saam Barati 2019-06-25 14:19:21 PDT
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?
Comment 6 Justin Fan 2019-06-25 15:04:18 PDT
(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.
Comment 7 Saam Barati 2019-06-25 15:31:02 PDT
Comment on attachment 372666 [details]
Patch

r=me
Comment 8 Justin Fan 2019-06-25 17:27:52 PDT
Created attachment 372884 [details]
Patch for landing
Comment 9 Justin Fan 2019-06-25 17:32:54 PDT
Created attachment 372885 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-06-25 18:40:59 PDT
Comment on attachment 372885 [details]
Patch for landing

Clearing flags on attachment: 372885

Committed r246824: <https://trac.webkit.org/changeset/246824>
Comment 11 WebKit Commit Bot 2019-06-25 18:41:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-06-26 16:32:56 PDT
<rdar://problem/52218035>
Comment 13 Radar WebKit Bug Importer 2019-06-26 16:32:57 PDT
<rdar://problem/52218036>