WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
WIP
(41.30 KB, patch)
2019-06-09 23:54 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(40.51 KB, patch)
2019-06-13 00:02 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(41.20 KB, patch)
2019-06-13 00:11 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(83.96 KB, patch)
2019-06-13 22:11 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(92.31 KB, patch)
2019-06-14 00:10 PDT
,
Myles C. Maxfield
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
WIP
(83.96 KB, patch)
2019-06-15 09:40 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(97.27 KB, patch)
2019-06-17 00:11 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(99.53 KB, patch)
2019-06-17 00:45 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(1.40 MB, patch)
2019-06-17 02:57 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
WIP
(1.42 MB, patch)
2019-06-17 19:20 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(1.42 MB, patch)
2019-06-18 13:56 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(125.68 KB, patch)
2019-06-18 19:48 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(126.80 KB, patch)
2019-06-19 10:11 PDT
,
Myles C. Maxfield
saam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2019-06-09 23:26:55 PDT
Created
attachment 371723
[details]
WIP
Myles C. Maxfield
Comment 2
2019-06-09 23:44:01 PDT
Fixes
https://bugs.webkit.org/show_bug.cgi?id=198306
Myles C. Maxfield
Comment 3
2019-06-09 23:54:39 PDT
Created
attachment 371725
[details]
WIP
Radar WebKit Bug Importer
Comment 4
2019-06-12 08:54:01 PDT
<
rdar://problem/51668841
>
Myles C. Maxfield
Comment 5
2019-06-13 00:02:52 PDT
Created
attachment 372024
[details]
WIP
Myles C. Maxfield
Comment 6
2019-06-13 00:11:04 PDT
Created
attachment 372027
[details]
WIP
Myles C. Maxfield
Comment 7
2019-06-13 22:11:02 PDT
Created
attachment 372105
[details]
WIP
Myles C. Maxfield
Comment 8
2019-06-14 00:10:39 PDT
Created
attachment 372109
[details]
WIP
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
Created
attachment 372195
[details]
WIP
Myles C. Maxfield
Comment 12
2019-06-17 00:11:42 PDT
Created
attachment 372234
[details]
WIP
Myles C. Maxfield
Comment 13
2019-06-17 00:45:43 PDT
Created
attachment 372235
[details]
WIP
Myles C. Maxfield
Comment 14
2019-06-17 02:57:58 PDT
Created
attachment 372238
[details]
Patch
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
Created
attachment 372316
[details]
WIP
Myles C. Maxfield
Comment 19
2019-06-18 13:56:24 PDT
Created
attachment 372378
[details]
WIP
Myles C. Maxfield
Comment 20
2019-06-18 19:48:15 PDT
Created
attachment 372421
[details]
Patch
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
Created
attachment 372470
[details]
Patch
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
Committed
r246631
: <
https://trac.webkit.org/changeset/246631
>
Saam Barati
Comment 31
2019-06-20 10:36:17 PDT
watchOS build fix landed in:
https://trac.webkit.org/changeset/246640/webkit
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug