Bug 199215 – [WHLSL] Matrix memory layout should match HLSL by laying out columns linearly

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

[Justin Fan]

From MSL docs: 

>// This sets the 4th element of the 3rd column to 3.0. 
>m[2][3] = 3.0f;

This is opposite of HLSL, which is what WHLSL should be compatible with. I don't think we are taking this into account when compiling WHLSL.

From Microsoft docs:

>A 4x4 matrix is accessed with the following indices:

>[0][0], [0][1], [0][2], [0][3]
>[1][0], [1][1], [1][2], [1][3]
>[2][0], [2][1], [2][2], [2][3]
>[3][0], [3][1], [3][2], [3][3]

[Justin Fan]

Myles' comments:

> 1) make the WHLSL functions round-trip. So if you invoke a bunch of different constructors and then run a bunch of getters, you get the expected thing.
> 2) make the memory layout match HLSL. If you upload 16 floats in a buffer and then tell the shader that the buffer contains a matrix, the getters should also return the expected thing

[Myles C. Maxfield]

In Direct3D, if you put [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16] in a buffer and tell the shader that the buffer is a matrix, and then ask the matrix for [0][1], the result is 5.

If you call float4x4(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16) and then you ask it for [0][1], the result is 2.

If you call float4x4(float4(1, 2, 3, 4), float4(5, 6, 7, 8), float4(9, 10, 11, 12), float4(13, 14, 15, 16)) and then you ask it for [0][1], the result is 2.

[Myles C. Maxfield]

In Direct3D, if you put [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32] in a buffer, and tell the shader that it's two float4x4s next to each other, then you mul(buffer1[0], buffer1[1]) and put the result in another buffer and then read the buffer on the CPU side, the results are:

538
612
686
760
650
740
830
920
762
868
974
1080
874
996
1118
1240

Thus, the two matrices are
[1 5  9 13]   [17 21 25 29]
[2 6 10 14] * [18 22 26 30]
[3 7 11 15]   [19 23 27 31]
[4 8 12 16]   [20 24 28 32]

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #3)
> In Direct3D, if you put [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
> 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32] in a
> buffer, and tell the shader that it's two float4x4s next to each other, then
> you mul(buffer1[0], buffer1[1]) and put the result in another buffer and
> then read the buffer on the CPU side, the results are:
> 
> 538
> 612
> 686
> 760
> 650
> 740
> 830
> 920
> 762
> 868
> 974
> 1080
> 874
> 996
> 1118
> 1240
> 
> Thus, the two matrices are
> [1 5  9 13]   [17 21 25 29]
> [2 6 10 14] * [18 22 26 30]
> [3 7 11 15]   [19 23 27 31]
> [4 8 12 16]   [20 24 28 32]

and buffer1[0][0] is [1 5 9 13]

[Justin Fan]

For Metal Shading Language:

There is no difference between supplying [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16] as a buffer to a shader and telling it to treat it as a float4x4, or using the float4x4() constructor with those values. Result[0] is [1, 2, 3, 4].

If you put [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32] in a buffer, and tell the shader that it's two float4x4s next to each other, the results of buffer1[0] * buffer1[1] are:

538
612
686
760
650
740
830
920
762
868
974
1080
874
996
1118
1240

Thus, the two matrices are
[1 5  9 13]   [17 21 25 29]
[2 6 10 14] * [18 22 26 30]
[3 7 11 15]   [19 23 27 31]
[4 8 12 16]   [20 24 28 32]

and buffer1[0][0] is [1, 2, 3, 4].

This makes sense, as MSL docs imply matrices are built AND stored as Arrays Of Columns Vectors.

Thus the difference remains: HLSL (and thus WHLSL) indexes matrices as Arrays of Row Vectors.

[Justin Fan]

The difference is that HLSL's float4x4 constructor and indexing operators behave in row-major order, but in memory in both languages, the values are in column-major order.

[Justin Fan]

To make things even more fun, currently in WHLSL, if you put [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32] in a buffer, and tell the shader that it's two float4x4s next to each other, the results of mul(buffer1[0], buffer1[1]) are:

250
260
270
280
618
644
670
696
986
1028
1070
1112
1354
1412
1470
1528

Implying that the matrices are:
[1   2  3  4]   [17 18 19 20]
[5   6  7  8] * [21 22 23 24]
[9  10 11 12]   [25 26 27 28]
[13 14 15 16]   [29 30 31 32]

[Justin Fan]

This means: this patch must ensure that WHLSL indexing behavior matches HLSL, if we are going with HLSL source-compatibility. The underlying memory layout should not need change.

[Justin Fan]

(In reply to Justin Fan from comment #8)
> This means: this patch must ensure that WHLSL indexing behavior matches
> HLSL, if we are going with HLSL source-compatibility. The underlying memory
> layout should not need change.

e.g. in WHLSL, result[0][0] gives [1, 2, 3, 4] when it should return [1, 5, 9, 13].

[Saam Barati]

patch forthcoming

[Saam Barati]

Created attachment 374068 [details]
patch

[Justin Fan]

Comment on attachment 374068 [details]
patch

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

> LayoutTests/webgpu/whlsl-matrix-memory-layout.html:5
> +<script src="../resources/js-test-pre.js"></script>

Any reason why you didn't use the WHLSL harness to write this test?

[Myles C. Maxfield]

Comment on attachment 374068 [details]
patch

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

> Source/WebCore/ChangeLog:11
> +        [0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15]

This is an odd way to structure this sentence. Why not say the memory is 1, 2, 3?

> Source/WebCore/ChangeLog:20
> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>

Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?

[Myles C. Maxfield]

Comment on attachment 374068 [details]
patch

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

I’ll wait for Saam to answer the question about why we’re using arrays instead of matrixes as the container before finishing the review.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:261
> +    auto numberOfMatrixColumns = [&] () -> unsigned {

Can we share some code?

[Saam Barati]

Comment on attachment 374068 [details]
patch

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

>> Source/WebCore/ChangeLog:11
>> +        [0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15]
> 
> This is an odd way to structure this sentence. Why not say the memory is 1, 2, 3?

I’m confused what you’re suggesting. I’m just trying to show what the backing memory of the below matrix is. Im not sure why how other backing store of memory as an example is any clearer. Maybe you have something in mind?

>> Source/WebCore/ChangeLog:20
>> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>
> 
> Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?

I’m confused what the alternative is. This patch is making it a container (specifically array) and we change the accessors to do the right thing

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:261
>> +    auto numberOfMatrixColumns = [&] () -> unsigned {
> 
> Can we share some code?

Yup. Can fix

>> LayoutTests/webgpu/whlsl-matrix-memory-layout.html:5
>> +<script src="../resources/js-test-pre.js"></script>
> 
> Any reason why you didn't use the WHLSL harness to write this test?

The main reason was just to manually control the buffer explicitly in the test. But it could be written using the harness

[Myles C. Maxfield]

Comment on attachment 374068 [details]
patch

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

>>> Source/WebCore/ChangeLog:11
>>> +        [0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15]
>> 
>> This is an odd way to structure this sentence. Why not say the memory is 1, 2, 3?
> 
> I’m confused what you’re suggesting. I’m just trying to show what the backing memory of the below matrix is. Im not sure why how other backing store of memory as an example is any clearer. Maybe you have something in mind?

This sentence is of the form “A [X] with the form [some complicated thing] should be the same as [this simple thing]”. I just mean that these sentences are usually of the form ”A [X] with the form [this simple thing] should be the same as [some complicated thing].”

>>> Source/WebCore/ChangeLog:20
>>> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>
>> 
>> Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?
> 
> I’m confused what the alternative is. This patch is making it a container (specifically array) and we change the accessors to do the right thing

Oh, I meant: why are WHLSL matrices represented as MSL nested arrays instead of being represented by MSL matrices? That seem more natural.

[Saam Barati]

Comment on attachment 374068 [details]
patch

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

>>>> Source/WebCore/ChangeLog:11
>>>> +        [0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15]
>>> 
>>> This is an odd way to structure this sentence. Why not say the memory is 1, 2, 3?
>> 
>> I’m confused what you’re suggesting. I’m just trying to show what the backing memory of the below matrix is. Im not sure why how other backing store of memory as an example is any clearer. Maybe you have something in mind?
> 
> This sentence is of the form “A [X] with the form [some complicated thing] should be the same as [this simple thing]”. I just mean that these sentences are usually of the form ”A [X] with the form [this simple thing] should be the same as [some complicated thing].”

I can change it.

>>>> Source/WebCore/ChangeLog:20
>>>> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>
>>> 
>>> Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?
>> 
>> I’m confused what the alternative is. This patch is making it a container (specifically array) and we change the accessors to do the right thing
> 
> Oh, I meant: why are WHLSL matrices represented as MSL nested arrays instead of being represented by MSL matrices? That seem more natural.

they’re not nested arrays. just a flat array.

[Myles C. Maxfield]

Comment on attachment 374068 [details]
patch

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

>>>>> Source/WebCore/ChangeLog:20
>>>>> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>
>>>> 
>>>> Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?
>>> 
>>> I’m confused what the alternative is. This patch is making it a container (specifically array) and we change the accessors to do the right thing
>> 
>> Oh, I meant: why are WHLSL matrices represented as MSL nested arrays instead of being represented by MSL matrices? That seem more natural.
> 
> they’re not nested arrays. just a flat array.

Oh, sorry, my bad. I meant “why represent a WHLSL matrix as a MSL array instead of as an MSL matrix?”

[Saam Barati]

Comment on attachment 374068 [details]
patch

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

>>>>>> Source/WebCore/ChangeLog:20
>>>>>> +        contents of a matrix. So the matrix float4x3 will now be an array<float, 12>
>>>>> 
>>>>> Why not just consider the matrix to be a container, and make the accessors/mutators do the right thing?
>>>> 
>>>> I’m confused what the alternative is. This patch is making it a container (specifically array) and we change the accessors to do the right thing
>>> 
>>> Oh, I meant: why are WHLSL matrices represented as MSL nested arrays instead of being represented by MSL matrices? That seem more natural.
>> 
>> they’re not nested arrays. just a flat array.
> 
> Oh, sorry, my bad. I meant “why represent a WHLSL matrix as a MSL array instead of as an MSL matrix?”

I think it’s less understandable to take a metal matrix and treat it as a bag of elements that needs to be accessed in a bizarre way. It’s simpler just to have our own bag of bytes and define our own way of indexing into it. It’s one level of weirdness instead of two.

[Saam Barati]

Created attachment 374162 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 374162 [details]
patch for landing

Clearing flags on attachment: 374162

Committed r247468: <https://trac.webkit.org/changeset/247468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53136686>