Bug 198704 – [WHLSL] Hook up common texture functions

• Component: WebGPU
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Just Sample(), Load(), Store(), and GetDimensions()

[Myles C. Maxfield]

Created attachment 371723 [details]
WIP

[Myles C. Maxfield]

Fixes https://bugs.webkit.org/show_bug.cgi?id=198306

[Myles C. Maxfield]

Created attachment 371725 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/51668841>

[Myles C. Maxfield]

Created attachment 372024 [details]
WIP

[Myles C. Maxfield]

Created attachment 372027 [details]
WIP

[Myles C. Maxfield]

Created attachment 372105 [details]
WIP

[Myles C. Maxfield]

Created attachment 372109 [details]
WIP

[EWS Watchlist]

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]

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]

Created attachment 372195 [details]
WIP

[Myles C. Maxfield]

Created attachment 372234 [details]
WIP

[Myles C. Maxfield]

Created attachment 372235 [details]
WIP

[Myles C. Maxfield]

Created attachment 372238 [details]
Patch

[EWS Watchlist]

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]

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 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]

Created attachment 372316 [details]
WIP

[Myles C. Maxfield]

Created attachment 372378 [details]
WIP

[Myles C. Maxfield]

Created attachment 372421 [details]
Patch

[Myles C. Maxfield]

(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]

*** Bug 198306 has been marked as a duplicate of this bug. ***

[EWS Watchlist]

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]

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]

Created attachment 372470 [details]
Patch

[Saam Barati]

*** Bug 198987 has been marked as a duplicate of this bug. ***

[Justin Fan]

r=me for the WebGPU bits.

[Saam Barati]

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 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]

Committed r246631: <https://trac.webkit.org/changeset/246631>

[Saam Barati]

watchOS build fix landed in:
https://trac.webkit.org/changeset/246640/webkit