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.
Created attachment 372666 [details] Patch
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
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
Test failure is a crash unrelated to this patch.
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?
(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 on attachment 372666 [details] Patch r=me
Created attachment 372884 [details] Patch for landing
Created attachment 372885 [details] Patch for landing
Comment on attachment 372885 [details] Patch for landing Clearing flags on attachment: 372885 Committed r246824: <https://trac.webkit.org/changeset/246824>
All reviewed patches have been landed. Closing bug.
<rdar://problem/52218035>
<rdar://problem/52218036>