Bug 198876 – [WHLSL] Support matrices

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Jon Lee]

[WHLSL] Support matrices

[Radar WebKit Bug Importer]

<rdar://problem/51768882>

[Saam Barati]

Gonna do this.

[Saam Barati]

Created attachment 372404 [details]
WIP

[Saam Barati]

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

[Saam Barati]

Created attachment 372409 [details]
patch

[EWS Watchlist]

Attachment 372409 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLMetalCodeGenerator.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 372409 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372409&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:629
> +native float3 operator[](float2x3, uint);
> +native float2x3 operator[]=(float2x3, uint, float3);

Why is this just for these two types?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:697
> +    float2x3 result;
> +    result[0][0] = a[0][0] - b;
> +    result[0][1] = a[0][1] - b;
> +    result[0][2] = a[0][2] - b;
> +    result[1][0] = a[1][0] - b;
> +    result[1][1] = a[1][1] - b;
> +    result[1][2] = a[1][2] - b;
> +    return result;

Why not just return operator+(a, -b)?

[Myles C. Maxfield]

Comment on attachment 372409 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372409&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:689
> +float2x3 operator-(float2x3 a, float b) {

Can you add mul() for two mat4s and for a mat4 & vec4?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:325
> +    SetForScope<Optional<BreakContext>> breakContext(m_currentBreakContext, BreakContext::Loop);

This is cool. You’re saving the chain of contexts on the stack instead of in a member Vector.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:414
>      // FIXME: https://bugs.webkit.org/show_bug.cgi?id=195812 Figure out whether we need to break or fallthrough.

Should this FIXME be deleted?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLMetalCodeGenerator.cpp:60
> +    if (dumpMetalCode) {

Cool.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:243
> +        stringBuilder.append(makeString("    ", metalReturnName, " vectorResult;\n"));

The result is always a vector, and all vectors have a scalar constructor, so you should be able to say “metalReturnName(0)”. That would be better than memset.

[Saam Barati]

Comment on attachment 372409 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372409&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:629
>> +native float2x3 operator[]=(float2x3, uint, float3);
> 
> Why is this just for these two types?

It was sufficient for testing the changes in this patch. I held off on adding more until we do this:
https://bugs.webkit.org/show_bug.cgi?id=198186

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:697
>> +    return result;
> 
> Why not just return operator+(a, -b)?

This is auto generated.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:414
>>      // FIXME: https://bugs.webkit.org/show_bug.cgi?id=195812 Figure out whether we need to break or fallthrough.
> 
> Should this FIXME be deleted?

I'm not sure. I think that should be considered as part of closing out that bug. This just allowed us to use switch in the stdlib assuming it's well formed.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:243
>> +        stringBuilder.append(makeString("    ", metalReturnName, " vectorResult;\n"));
> 
> The result is always a vector, and all vectors have a scalar constructor, so you should be able to say “metalReturnName(0)”. That would be better than memset.

sounds good.

[Saam Barati]

Comment on attachment 372409 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372409&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:689
>> +float2x3 operator-(float2x3 a, float b) {
> 
> Can you add mul() for two mat4s and for a mat4 & vec4?

Will add.

[Saam Barati]

Created attachment 372418 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 372418 [details]
patch for landing

Clearing flags on attachment: 372418

Committed r246579: <https://trac.webkit.org/changeset/246579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.