Bug 198704 - [WHLSL] Hook up common texture functions
Summary: [WHLSL] Hook up common texture functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 198306 198987 (view as bug list)
Depends on: 198706
Blocks: 195813
  Show dependency treegraph
 
Reported: 2019-06-09 23:25 PDT by Myles C. Maxfield
Modified: 2019-06-20 10:36 PDT (History)
13 users (show)

See Also:


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: 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, Build Bot
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, Build Bot
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, Build Bot
no flags Details
Patch (126.80 KB, patch)
2019-06-19 10:11 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-06-09 23:25:13 PDT
Just Sample(), Load(), Store(), and GetDimensions()
Comment 1 Myles C. Maxfield 2019-06-09 23:26:55 PDT
Created attachment 371723 [details]
WIP
Comment 2 Myles C. Maxfield 2019-06-09 23:44:01 PDT
Fixes https://bugs.webkit.org/show_bug.cgi?id=198306
Comment 3 Myles C. Maxfield 2019-06-09 23:54:39 PDT
Created attachment 371725 [details]
WIP
Comment 4 Radar WebKit Bug Importer 2019-06-12 08:54:01 PDT
<rdar://problem/51668841>
Comment 5 Myles C. Maxfield 2019-06-13 00:02:52 PDT
Created attachment 372024 [details]
WIP
Comment 6 Myles C. Maxfield 2019-06-13 00:11:04 PDT
Created attachment 372027 [details]
WIP
Comment 7 Myles C. Maxfield 2019-06-13 22:11:02 PDT
Created attachment 372105 [details]
WIP
Comment 8 Myles C. Maxfield 2019-06-14 00:10:39 PDT
Created attachment 372109 [details]
WIP
Comment 9 Build Bot 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.
Comment 10 Build Bot 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
Comment 11 Myles C. Maxfield 2019-06-15 09:40:20 PDT
Created attachment 372195 [details]
WIP
Comment 12 Myles C. Maxfield 2019-06-17 00:11:42 PDT
Created attachment 372234 [details]
WIP
Comment 13 Myles C. Maxfield 2019-06-17 00:45:43 PDT
Created attachment 372235 [details]
WIP
Comment 14 Myles C. Maxfield 2019-06-17 02:57:58 PDT
Created attachment 372238 [details]
Patch
Comment 15 Build Bot 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.
Comment 16 Build Bot 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
Comment 17 Sam Weinig 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.
Comment 18 Myles C. Maxfield 2019-06-17 19:20:51 PDT
Created attachment 372316 [details]
WIP
Comment 19 Myles C. Maxfield 2019-06-18 13:56:24 PDT
Created attachment 372378 [details]
WIP
Comment 20 Myles C. Maxfield 2019-06-18 19:48:15 PDT
Created attachment 372421 [details]
Patch
Comment 21 Myles C. Maxfield 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.
Comment 22 Myles C. Maxfield 2019-06-18 19:57:35 PDT
*** Bug 198306 has been marked as a duplicate of this bug. ***
Comment 23 Build Bot 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
Comment 24 Build Bot 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
Comment 25 Myles C. Maxfield 2019-06-19 10:11:59 PDT
Created attachment 372470 [details]
Patch
Comment 26 Saam Barati 2019-06-19 13:07:34 PDT
*** Bug 198987 has been marked as a duplicate of this bug. ***
Comment 27 Justin Fan 2019-06-19 15:31:14 PDT
r=me for the WebGPU bits.
Comment 28 Saam Barati 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.
Comment 29 Myles C. Maxfield 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.
Comment 30 Myles C. Maxfield 2019-06-20 02:41:18 PDT
Committed r246631: <https://trac.webkit.org/changeset/246631>
Comment 31 Saam Barati 2019-06-20 10:36:17 PDT
watchOS build fix landed in:
https://trac.webkit.org/changeset/246640/webkit