[SPIR-V] ByteAddressBuffer doesn't emit RowMaj or ColMaj decorations for templated loads

[Jasper-Bekkers]

I'm not sure if this is done explicitly or not, however when converting some of our ConstantBuffer's to ByteAddressBuffers we noticed that Load doesn't OpDecorate the matrix it's loading, nor do column_major or row_major attributes get respected when used like this: .Load<column_major float4x4>(...).

Example: http://shader-playground.timjones.io/e8c49c3276e111e220873d91c937bb2b

I'm not sure if it's intentional or not, but at least I wanted to raise the issue that it's inconsistent.

[dneto0]

Looking at the generated SPIR-V, the matrix type is only used for an intermediate value. It's not used to describe the contents of a buffer. So there is no obligation to apply layout decorations to it. In fact, it should not have layout decorations.

I think this is not a bug. I recommend closing.

[MarijnS95]

@dneto0 Note that the loads should probably get vectorized as per #3372, then this becomes relevant?

Generated SPIR-V remains identical regardless of column_major or row_major annotations, which should be a bug if I'm not mistaken? Even if the layout decoration is "removed" by the compiler in "favour" of "raw" handling of the matrix as seems to be the case here, all these operations should be transposed when matrix order is changed?

[Jasper-Bekkers]

Looking at the generated SPIR-V, the matrix type is only used for an intermediate value. It's not used to describe the contents of a buffer. So there is no obligation to apply layout decorations to it. In fact, it should not have layout decorations.

Notice that not doing this leads to subtle SPIR-V specific bugs when converting for example from a StructuredBuffer to a ByteAddressBuffer. Though this could also point to a codegen bug for the SPIRV path that just loads the matrices in the incorrect order.

[gjaegy]

Hi, I'm actually facing the exact same issue...
We are also switching to bindless and all our matrices are now stored into a ByteAdressBuffer. When loading them from the buffer (using load()), an additional transpose() call is required, which I really want to avoir.
May I ask, what is the status of that issue ? Is it a bug or not, will it be fixed or not ?
Thanks a lot !

[jbmcgee]

Also ran into this issue. Matrices are stored column-major in ByteAddressBuffer, but the load results in row-major.

Looking at the generated SPIR-V, the matrix type is only used for an intermediate value. It's not used to describe the contents of a buffer. So there is no obligation to apply layout decorations to it. In fact, it should not have layout decorations.

I think this is not a bug. I recommend closing.

From looking at the spirv, think the issue is:
%87 = OpCompositeConstruct %mat4v4float %47 %60 %73 %86.
I expected column-major but internal compiler decides row-major.

[kuhar]

cc @kuhar

[kuhar]

Hi @Jasper-Bekkers, @gjaegy, @MarijnS95, and @jbmcgee,

I looked into this issue and came up with a draft PR: #4526.
However, the correct semantics are not clear to me:

Generated SPIR-V remains identical regardless of column_major or row_major annotations, which should be a bug if I'm not mistaken? Even if the layout decoration is "removed" by the compiler in "favour" of "raw" handling of the matrix as seems to be the case here, all these operations should be transposed when matrix order is changed?

I compiled the example from the first comment to DXIL, annotating the load with both row_major and column_major and the final DXIL seems to be identical. Shouldn't DXIL be affected as well, or am I holding it wrong?

Matrices are stored column-major in ByteAddressBuffer, but the load results in row-major.

We are also switching to bindless and all our matrices are now stored into a ByteAdressBuffer. When loading them from the buffer (using load()), an additional transpose() call is required, which I really want to avoir.

Do you know of some documentation that states how matrices are stored in ByteAddressBuffers?

[kuhar]

I checked with @chaoticbob who suggested we introduce a new flag to control the default matrix order.

I updated the PR in the following way:
The new flag is off by default, so the default order remains what it was, so that we don't break existing shaders. To change the behavior, you can either annotate loads with column_major or set -fspv-use-dx-buffer-matrix-order globally.

[kuhar]

I checked with @chaoticbob who suggested we introduce a new flag to control the default matrix order.

I updated the PR in the following way: The new flag is off by default, so the default order remains what it was, so that we don't break existing shaders. To change the behavior, you can either annotate loads with column_major or set -fspv-use-dx-buffer-matrix-order globally.

Update: I further checked with @tex3d. We will keep the behavior consistent with HLSL/DXIL and assume the matrix order in raw buffers to be column major, and provide a compatibility flag to opt-out of the change: -fspv-use-legacy-buffer-matrix-order. We won't honor column_major and row_major attributes in .Load/.Store because these are not currently supported in the DXIL codegen either.