Bug 198876 - [WHLSL] Support matrices
Summary: [WHLSL] Support matrices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 198313 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-14 18:02 PDT by Jon Lee
Modified: 2019-06-18 19:36 PDT (History)
7 users (show)

See Also:


Attachments
WIP (17.82 KB, patch)
2019-06-18 16:32 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (21.72 KB, patch)
2019-06-18 17:07 PDT, Saam Barati
dino: review+
Details | Formatted Diff | Diff
patch for landing (32.87 KB, patch)
2019-06-18 18:53 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2019-06-14 18:02:56 PDT
[WHLSL] Support matrices
Comment 1 Radar WebKit Bug Importer 2019-06-14 18:04:20 PDT
<rdar://problem/51768882>
Comment 2 Saam Barati 2019-06-18 12:38:48 PDT
Gonna do this.
Comment 3 Saam Barati 2019-06-18 16:32:04 PDT
Created attachment 372404 [details]
WIP
Comment 4 Saam Barati 2019-06-18 16:41:06 PDT
*** Bug 198313 has been marked as a duplicate of this bug. ***
Comment 5 Saam Barati 2019-06-18 17:07:48 PDT
Created attachment 372409 [details]
patch
Comment 6 EWS Watchlist 2019-06-18 17:10:53 PDT
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.
Comment 7 Dean Jackson 2019-06-18 17:14:55 PDT
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)?
Comment 8 Myles C. Maxfield 2019-06-18 17:19:04 PDT
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.
Comment 9 Saam Barati 2019-06-18 17:53:52 PDT
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.
Comment 10 Saam Barati 2019-06-18 18:47:45 PDT
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.
Comment 11 Saam Barati 2019-06-18 18:53:49 PDT
Created attachment 372418 [details]
patch for landing
Comment 12 WebKit Commit Bot 2019-06-18 19:36:49 PDT
Comment on attachment 372418 [details]
patch for landing

Clearing flags on attachment: 372418

Committed r246579: <https://trac.webkit.org/changeset/246579>
Comment 13 WebKit Commit Bot 2019-06-18 19:36:50 PDT
All reviewed patches have been landed.  Closing bug.