RESOLVED FIXED 198704
[WHLSL] Hook up common texture functions
https://bugs.webkit.org/show_bug.cgi?id=198704
Summary [WHLSL] Hook up common texture functions
Myles C. Maxfield
Reported 2019-06-09 23:25:13 PDT
Just Sample(), Load(), Store(), and GetDimensions()
Attachments
WIP (71.92 KB, patch)
2019-06-09 23:26 PDT, Myles C. Maxfield
no flags
WIP (41.30 KB, patch)
2019-06-09 23:54 PDT, Myles C. Maxfield
no flags
WIP (40.51 KB, patch)
2019-06-13 00:02 PDT, Myles C. Maxfield
no flags
WIP (41.20 KB, patch)
2019-06-13 00:11 PDT, Myles C. Maxfield
no flags
WIP (83.96 KB, patch)
2019-06-13 22:11 PDT, Myles C. Maxfield
no flags
WIP (92.31 KB, patch)
2019-06-14 00:10 PDT, Myles C. Maxfield
ews-watchlist: commit-queue-
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.52 MB, application/zip)
2019-06-14 01:18 PDT, EWS Watchlist
no flags
WIP (83.96 KB, patch)
2019-06-15 09:40 PDT, Myles C. Maxfield
no flags
WIP (97.27 KB, patch)
2019-06-17 00:11 PDT, Myles C. Maxfield
no flags
WIP (99.53 KB, patch)
2019-06-17 00:45 PDT, Myles C. Maxfield
no flags
Patch (1.40 MB, patch)
2019-06-17 02:57 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.83 MB, application/zip)
2019-06-17 04:07 PDT, EWS Watchlist
no flags
WIP (1.42 MB, patch)
2019-06-17 19:20 PDT, Myles C. Maxfield
no flags
WIP (1.42 MB, patch)
2019-06-18 13:56 PDT, Myles C. Maxfield
no flags
Patch (125.68 KB, patch)
2019-06-18 19:48 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.91 MB, application/zip)
2019-06-18 20:43 PDT, EWS Watchlist
no flags
Patch (126.80 KB, patch)
2019-06-19 10:11 PDT, Myles C. Maxfield
saam: review+
Myles C. Maxfield
Comment 1 2019-06-09 23:26:55 PDT
Myles C. Maxfield
Comment 2 2019-06-09 23:44:01 PDT
Myles C. Maxfield
Comment 3 2019-06-09 23:54:39 PDT
Radar WebKit Bug Importer
Comment 4 2019-06-12 08:54:01 PDT
Myles C. Maxfield
Comment 5 2019-06-13 00:02:52 PDT
Myles C. Maxfield
Comment 6 2019-06-13 00:11:04 PDT
Myles C. Maxfield
Comment 7 2019-06-13 22:11:02 PDT
Myles C. Maxfield
Comment 8 2019-06-14 00:10:39 PDT
EWS Watchlist
Comment 9 2019-06-14 01:18:54 PDT
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.
EWS Watchlist
Comment 10 2019-06-14 01:18:56 PDT
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
Myles C. Maxfield
Comment 11 2019-06-15 09:40:20 PDT
Myles C. Maxfield
Comment 12 2019-06-17 00:11:42 PDT
Myles C. Maxfield
Comment 13 2019-06-17 00:45:43 PDT
Myles C. Maxfield
Comment 14 2019-06-17 02:57:58 PDT
EWS Watchlist
Comment 15 2019-06-17 04:07:39 PDT
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.
EWS Watchlist
Comment 16 2019-06-17 04:07:41 PDT
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
Sam Weinig
Comment 17 2019-06-17 13:04:35 PDT
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.
Myles C. Maxfield
Comment 18 2019-06-17 19:20:51 PDT
Myles C. Maxfield
Comment 19 2019-06-18 13:56:24 PDT
Myles C. Maxfield
Comment 20 2019-06-18 19:48:15 PDT
Myles C. Maxfield
Comment 21 2019-06-18 19:49:16 PDT
(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.
Myles C. Maxfield
Comment 22 2019-06-18 19:57:35 PDT
*** Bug 198306 has been marked as a duplicate of this bug. ***
EWS Watchlist
Comment 23 2019-06-18 20:43:29 PDT
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
EWS Watchlist
Comment 24 2019-06-18 20:43:31 PDT
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
Myles C. Maxfield
Comment 25 2019-06-19 10:11:59 PDT
Saam Barati
Comment 26 2019-06-19 13:07:34 PDT
*** Bug 198987 has been marked as a duplicate of this bug. ***
Justin Fan
Comment 27 2019-06-19 15:31:14 PDT
r=me for the WebGPU bits.
Saam Barati
Comment 28 2019-06-19 20:24:01 PDT
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.
Myles C. Maxfield
Comment 29 2019-06-20 02:05:15 PDT
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.
Myles C. Maxfield
Comment 30 2019-06-20 02:41:18 PDT
Saam Barati
Comment 31 2019-06-20 10:36:17 PDT
Note You need to log in before you can comment on or make changes to this bug.