Just Sample(), Load(), Store(), and GetDimensions()
Created attachment 371723 [details] WIP
Fixes https://bugs.webkit.org/show_bug.cgi?id=198306
Created attachment 371725 [details] WIP
<rdar://problem/51668841>
Created attachment 372024 [details] WIP
Created attachment 372027 [details] WIP
Created attachment 372105 [details] WIP
Created attachment 372109 [details] WIP
Comment on attachment 372109 [details] WIP Attachment 372109 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12472096 Number of test failures exceeded the failure limit.
Created attachment 372115 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 372195 [details] WIP
Created attachment 372234 [details] WIP
Created attachment 372235 [details] WIP
Created attachment 372238 [details] Patch
Comment on attachment 372238 [details] Patch Attachment 372238 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12495955 Number of test failures exceeded the failure limit.
Created attachment 372239 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372238&action=review > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:157 > + stringBuilder.append(makeString(" ", addressSpace, " ", mangledTypeName, "* ", elementName, " [[id(", index, ")]];\n")); This does an unnecessary extra String allocation due tot he makeString(). It would be more efficient to call append() multiple times on the StringBuilder for each component. We should consider adding a function to StringBuilder which emulates the makeString() syntax and allows multiple "string-like" parameters.
Created attachment 372316 [details] WIP
Created attachment 372378 [details] WIP
Created attachment 372421 [details] Patch
(In reply to Sam Weinig from comment #17) > Comment on attachment 372238 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372238&action=review > > > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:157 > > + stringBuilder.append(makeString(" ", addressSpace, " ", mangledTypeName, "* ", elementName, " [[id(", index, ")]];\n")); > > This does an unnecessary extra String allocation due tot he makeString(). It > would be more efficient to call append() multiple times on the StringBuilder > for each component. We should consider adding a function to StringBuilder > which emulates the makeString() syntax and allows multiple "string-like" > parameters. This is a good idea. I'd like to do it in a follow-up, though.
*** Bug 198306 has been marked as a duplicate of this bug. ***
Comment on attachment 372421 [details] Patch Attachment 372421 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12516009 New failing tests: webgpu/whlsl-duplicate-types-should-not-produce-duplicate-ctors.html webgpu/whlsl-buffer-length.html
Created attachment 372423 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 372470 [details] Patch
*** Bug 198987 has been marked as a duplicate of this bug. ***
r=me for the WebGPU bits.
Comment on attachment 372470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372470&action=review r=me > Source/WebCore/ChangeLog:16 > + Reviewed by NOBODY (OOPS!). I think you want this above the description above. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:112 > + Visitor::visit(expression); > + Visitor::visit(expression.resolvedType()); Is this actually needed? How would an expression end up with this type without us visiting that type in some other way? If it is needed, I think you should be calling checkErrorAndVisit here instead. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:141 > + if (newNameResolver.error()) > + setError(); Why not do it in the destructor instead of copy pasting it everywhere we construct a nested name resolver? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeConstructors.cpp:85 > + virtual ~FindAllTypes() = default; 👍🏼 > Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeConstructors.cpp:185 > + if (nativeTypeDeclaration.isTexture() || matches(nativeTypeDeclaration, program.intrinsics().samplerType())) nit: You have this check in a few places throughout this patch. Maybe make a helper method like "isTextureOrSampler" (with some better name)? > Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:69 > + LOG(WebGPU, "%s: Texture cannot have OUTPUT_ATTACHMENT usage with STORAGE or SAMPLED usages!", functionName); Is this the kind of thing that is like an assert, or can user code trigger this? Is it worth logging? > LayoutTests/webgpu/whlsl-textures-getdimensions.html:14 > + GetDimensions(theTexture, 0, &width, &height, &numberOfLevels); I think it's worth addressing this ignored mipLevel in some way. Maybe either: 1. Removing it as a parameter in WHLSL. 2. Ensuring it's always a constant literal. 3. Filing a bug to do something above or something else in the future.
Comment on attachment 372470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372470&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:112 >> + Visitor::visit(expression.resolvedType()); > > Is this actually needed? How would an expression end up with this type without us visiting that type in some other way? > > If it is needed, I think you should be calling checkErrorAndVisit here instead. It's needed because user code can say &myTexture, which would create a pointer to a texture without the type itself appearing anywhere in the text of the program. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:141 >> + setError(); > > Why not do it in the destructor instead of copy pasting it everywhere we construct a nested name resolver? Good idea! >> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:69 >> + LOG(WebGPU, "%s: Texture cannot have OUTPUT_ATTACHMENT usage with STORAGE or SAMPLED usages!", functionName); > > Is this the kind of thing that is like an assert, or can user code trigger this? Is it worth logging? User code can trigger this. >> LayoutTests/webgpu/whlsl-textures-getdimensions.html:14 >> + GetDimensions(theTexture, 0, &width, &height, &numberOfLevels); > > I think it's worth addressing this ignored mipLevel in some way. Maybe either: > 1. Removing it as a parameter in WHLSL. > 2. Ensuring it's always a constant literal. > 3. Filing a bug to do something above or something else in the future. Turns out this argument is only required to be 0 for 1d textures. Because of that, I've hooked it up for all the other texture types, and removed the argument for 1d textures.
Committed r246631: <https://trac.webkit.org/changeset/246631>
watchOS build fix landed in: https://trac.webkit.org/changeset/246640/webkit