RESOLVED FIXED 199028
[WHLSL] Make whlsl-test-harness actually generate WHLSL shaders by default
https://bugs.webkit.org/show_bug.cgi?id=199028
Summary [WHLSL] Make whlsl-test-harness actually generate WHLSL shaders by default
Justin Fan
Reported 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.
Attachments
Patch (41.83 KB, patch)
2019-06-21 19:05 PDT, Justin Fan
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.75 MB, application/zip)
2019-06-21 21:05 PDT, EWS Watchlist
no flags
Patch for landing (42.75 KB, patch)
2019-06-25 17:27 PDT, Justin Fan
no flags
Patch for landing (42.83 KB, patch)
2019-06-25 17:32 PDT, Justin Fan
no flags
Justin Fan
Comment 1 2019-06-21 19:05:34 PDT
EWS Watchlist
Comment 2 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
EWS Watchlist
Comment 3 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
Justin Fan
Comment 4 2019-06-24 11:48:36 PDT
Test failure is a crash unrelated to this patch.
Saam Barati
Comment 5 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?
Justin Fan
Comment 6 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.
Saam Barati
Comment 7 2019-06-25 15:31:02 PDT
Comment on attachment 372666 [details] Patch r=me
Justin Fan
Comment 8 2019-06-25 17:27:52 PDT
Created attachment 372884 [details] Patch for landing
Justin Fan
Comment 9 2019-06-25 17:32:54 PDT
Created attachment 372885 [details] Patch for landing
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2019-06-25 18:41:01 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2019-06-26 16:32:56 PDT
Radar WebKit Bug Importer
Comment 13 2019-06-26 16:32:57 PDT
Note You need to log in before you can comment on or make changes to this bug.