Open cwfitzgerald opened 1 month ago
The bug may be due to the struct contains no other fields before the unsized array.
In this case slang should not be generating that struct at all, and should just turn a pointer to the struct type into a pointer to the array element type.
The workaround for now is to use pointer directly (uint8_t*), or add some field before the array.
@csyonghe I'm still getting this after upgrading to v2024.14.5. SPIRV generated for following sample is rejected by Mesa with a validation error.
Adding a preceding field to the struct fixes the validation error, but the array is still misaligned and accessed elements are garbled. Changing to a bounded array fixes both issues.
struct MeshStorage {
uint64_t QuadData[];
};
struct DispatchParams {
MeshStorage* Mesh;
uint64_t* Dest;
};
[vk::push_constant] DispatchParams pc;
[numthreads(64)]
void ComputeMain(uint tid: SV_DispatchThreadID) {
pc.Dest[tid] = pc.Mesh->QuadData[tid];
}
spirv-val test.spv --target-env vulkan1.3 --scalar-block-layout
error: line 28: Structure id 11 decorated as Block for variable in PushConstant storage class must follow scalar storage buffer layout rules: member 1 at offset 0 overlaps previous member ending at offset 7
%DispatchParams_natural = OpTypeStruct %_ptr_PhysicalStorageBuffer_uint %_ptr_PhysicalStorageBuffer_ulong
I can take another look into this, but is there any reason to use this kind of wrapper struct instead of declaring uint64_t* DispatchParams::Mesh
directly? After all the compiler will remove this struct and just turn Mesh into a uint64*. There may still be bugs around the handling of trailing unsized arrays.
I don't have a use-case for that scenario, this was for a minimal repro only. My original struct contains other fields before the array, but reading from it still produces garbled values due to the alignment issue.
With SPIRV-Tools v2024.4 v2024.4.rc1-0-g6dcc7e35, I am not able to reproduce this validation error. What version of spirv-val are you using? This is the spirv I get from the shader you posted, and it passes all validation with the spirv-val command line you posted.
; SPIR-V
; Version: 1.5
; Generator: Khronos; 40
; Bound: 32
; Schema: 0
OpCapability PhysicalStorageBufferAddresses
OpCapability Int64
OpCapability Shader
OpExtension "SPV_KHR_physical_storage_buffer"
OpMemoryModel PhysicalStorageBuffer64 GLSL450
OpEntryPoint GLCompute %ComputeMain "main" %pc %gl_GlobalInvocationID
OpExecutionMode %ComputeMain LocalSize 64 1 1
; Debug Information
OpSource Slang 1
OpName %DispatchParams_std140 "DispatchParams_std140" ; id %11
OpMemberName %DispatchParams_std140 0 "Mesh"
OpMemberName %DispatchParams_std140 1 "Dest"
OpName %pc "pc" ; id %16
OpName %ComputeMain "ComputeMain" ; id %2
; Annotations
OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
OpDecorate %_ptr_PhysicalStorageBuffer_uint ArrayStride 0
OpDecorate %_ptr_PhysicalStorageBuffer_ulong ArrayStride 8
OpDecorate %DispatchParams_std140 Block
OpMemberDecorate %DispatchParams_std140 0 Offset 0
OpMemberDecorate %DispatchParams_std140 1 Offset 8
; Types, variables and constants
%void = OpTypeVoid
%3 = OpTypeFunction %void
%uint = OpTypeInt 32 0
%v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
%_ptr_PhysicalStorageBuffer_uint = OpTypePointer PhysicalStorageBuffer %uint ; ArrayStride 0
%ulong = OpTypeInt 64 0
%_ptr_PhysicalStorageBuffer_ulong = OpTypePointer PhysicalStorageBuffer %ulong ; ArrayStride 8
%DispatchParams_std140 = OpTypeStruct %_ptr_PhysicalStorageBuffer_uint %_ptr_PhysicalStorageBuffer_ulong ; Block
%_ptr_PushConstant_DispatchParams_std140 = OpTypePointer PushConstant %DispatchParams_std140
%int = OpTypeInt 32 1
%int_1 = OpConstant %int 1
%_ptr_PushConstant__ptr_PhysicalStorageBuffer_ulong = OpTypePointer PushConstant %_ptr_PhysicalStorageBuffer_ulong
%int_0 = OpConstant %int 0
%_ptr_PushConstant__ptr_PhysicalStorageBuffer_uint = OpTypePointer PushConstant %_ptr_PhysicalStorageBuffer_uint
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input ; BuiltIn GlobalInvocationId
%pc = OpVariable %_ptr_PushConstant_DispatchParams_std140 PushConstant
; Function ComputeMain
%ComputeMain = OpFunction %void None %3
%4 = OpLabel
%7 = OpLoad %v3uint %gl_GlobalInvocationID
%10 = OpCompositeExtract %uint %7 0
%20 = OpAccessChain %_ptr_PushConstant__ptr_PhysicalStorageBuffer_ulong %pc %int_1
%21 = OpLoad %_ptr_PhysicalStorageBuffer_ulong %20
%22 = OpPtrAccessChain %_ptr_PhysicalStorageBuffer_ulong %21 %10
%25 = OpAccessChain %_ptr_PushConstant__ptr_PhysicalStorageBuffer_uint %pc %int_0
%26 = OpLoad %_ptr_PhysicalStorageBuffer_uint %25
%27 = OpBitcast %_ptr_PhysicalStorageBuffer_ulong %26
%28 = OpPtrAccessChain %_ptr_PhysicalStorageBuffer_ulong %27 %10
%29 = OpLoad %ulong %28 Aligned 8
OpStore %22 %29 Aligned 8
OpReturn
OpFunctionEnd
@dubiousconst282 When "it is producing garbled values", what fields are defined before the array?
Please note that since the array's element type is uint64_t, it will always be padded to the next 8-byte boundary. So if you have something like int count
as the first field, the first element is still going to start at byte offset 8.
I've tried adding different types of fields before the array, and so far I am not able to see any invalid output with top of tree Slang.
In my test, the exact struct looks like this:
struct MeshStorage {
int foo;
uint64_t QuadData[];
};
Running the previous shader with a buffer filled with sequence 0..n and reading the output, results in the following (I can provide a buildable sample if required).
Output with unbounded array:
0 1 2 3 4 5 6 7 8 9 a b c d e f
200000000 300000000 400000000 500000000 600000000 700000000 800000000 900000000 a00000000 b00000000 c00000000 d00000000 e00000000 f00000000 1000000000 1100000000
Output with uint64_t QuadData[1];
:
0 1 2 3 4 5 6 7 8 9 a b c d e f
1 2 3 4 5 6 7 8 9 a b c d e f 10
Relevant SPIR-V diff:
diff --git a/test_unbound.txt b/test_fixed.txt
index f5e61f2..c4459d4 100755
--- a/test_unbound.txt
+++ b/test_fixed.txt
@@ -1,7 +1,7 @@
; SPIR-V
; Version: 1.6
; Generator: Khronos; 40
-; Bound: 37
+; Bound: 38
; Schema: 0
OpCapability PhysicalStorageBufferAddresses
OpCapability Int64
@@ -16,15 +16,23 @@
OpMemberName %DispatchParams_natural 1 "Dest"
OpName %pc "pc"
OpName %ComputeMain "ComputeMain"
+ OpName %_Array_natural_uint641 "_Array_natural_uint641"
+ OpMemberName %_Array_natural_uint641 0 "data"
OpName %MeshStorage_natural "MeshStorage_natural"
OpMemberName %MeshStorage_natural 0 "foo"
+ OpMemberName %MeshStorage_natural 1 "QuadData"
OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
- OpDecorate %_ptr_PhysicalStorageBuffer_MeshStorage_natural ArrayStride 4
+ OpDecorate %_ptr_PhysicalStorageBuffer_MeshStorage_natural ArrayStride 16
OpDecorate %_ptr_PhysicalStorageBuffer_ulong ArrayStride 8
OpDecorate %DispatchParams_natural Block
OpMemberDecorate %DispatchParams_natural 0 Offset 0
OpMemberDecorate %DispatchParams_natural 1 Offset 8
+ OpDecorate %_ptr_PhysicalStorageBuffer__Array_natural_uint641 ArrayStride 8
+ OpDecorate %_arr_ulong_int_1 ArrayStride 8
+ OpDecorate %_ptr_PhysicalStorageBuffer__arr_ulong_int_1 ArrayStride 8
+ OpMemberDecorate %_Array_natural_uint641 0 Offset 0
OpMemberDecorate %MeshStorage_natural 0 Offset 0
+ OpMemberDecorate %MeshStorage_natural 1 Offset 8
%void = OpTypeVoid
%3 = OpTypeFunction %void
%uint = OpTypeInt 32 0
@@ -40,9 +48,13 @@
%_ptr_PushConstant__ptr_PhysicalStorageBuffer_ulong = OpTypePointer PushConstant %_ptr_PhysicalStorageBuffer_ulong
%int_0 = OpConstant %int 0
%_ptr_PushConstant_13 = OpTypePointer PushConstant %_ptr_PhysicalStorageBuffer_MeshStorage_natural
- %ulong_8 = OpConstant %ulong 8
-%MeshStorage_natural = OpTypeStruct %int
+ OpTypeForwardPointer %_ptr_PhysicalStorageBuffer__Array_natural_uint641 PhysicalStorageBuffer
+%_arr_ulong_int_1 = OpTypeArray %ulong %int_1
+%_ptr_PhysicalStorageBuffer__arr_ulong_int_1 = OpTypePointer PhysicalStorageBuffer %_arr_ulong_int_1
+%_Array_natural_uint641 = OpTypeStruct %_arr_ulong_int_1
+%MeshStorage_natural = OpTypeStruct %int %_Array_natural_uint641
%_ptr_PhysicalStorageBuffer_MeshStorage_natural = OpTypePointer PhysicalStorageBuffer %MeshStorage_natural
+%_ptr_PhysicalStorageBuffer__Array_natural_uint641 = OpTypePointer PhysicalStorageBuffer %_Array_natural_uint641
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
%pc = OpVariable %_ptr_PushConstant_DispatchParams_natural PushConstant
%ComputeMain = OpFunction %void None %3
@@ -54,12 +66,10 @@
%23 = OpPtrAccessChain %_ptr_PhysicalStorageBuffer_ulong %22 %10
%26 = OpAccessChain %_ptr_PushConstant_13 %pc %int_0
%27 = OpLoad %_ptr_PhysicalStorageBuffer_MeshStorage_natural %26
- %28 = OpPtrAccessChain %_ptr_PhysicalStorageBuffer_MeshStorage_natural %27 %int_1
- %29 = OpBitcast %ulong %28
- %30 = OpIAdd %ulong %29 %ulong_8
- %32 = OpBitcast %_ptr_PhysicalStorageBuffer_ulong %30
- %33 = OpPtrAccessChain %_ptr_PhysicalStorageBuffer_ulong %32 %10
- %34 = OpLoad %ulong %33 Aligned 8
- OpStore %23 %34 Aligned 8
+ %30 = OpAccessChain %_ptr_PhysicalStorageBuffer__Array_natural_uint641 %27 %int_1
+ %33 = OpAccessChain %_ptr_PhysicalStorageBuffer__arr_ulong_int_1 %30 %int_0
+ %34 = OpAccessChain %_ptr_PhysicalStorageBuffer_ulong %33 %10
+ %35 = OpLoad %ulong %34 Aligned 8
+ OpStore %23 %35 Aligned 8
OpReturn
OpFunctionEnd
It seems the array is getting removed from the struct, which causes stride to be changed to 4.
The fact that MeshStorage type itself having an array stride of 4 shouldn’t be a problem because it will never actually be used. The code should never be accessing Mesh[i]->xxx, since MeshStorage is unsized and doesn’t really have a valid stride.
Please note that if MeshStorage is unsized (having an unsized array as last element), and if you have a pointer to MeshStorage like MeshStorage* ptr, then ptr[i] is illegal code if i !=0.
We haven’t implemented the check yet but this code should result in a compile error.
@dubiousconst282 is this still an issue for you?
If you need an array to MeshStorage
, my suggestion is forMeshStorage
to define a uint64_t*
field instead of a uint64_t[]
.
That will turn MeshStorage
itself into a sized type which will allow MeshStorage* ptr
to be indexed.
I believe there has been a misunderstanding, the issue isn't in indexing the pointer Mesh[i]->data
, but the array itself Mesh->data[i]
, that generates wrong values when defined as unsized.
Regardless, I have worked around the issue by defining a sized array like aforementioned to minimize indirections, so it's a non-blocker for me. I'd still like to see it fixed though, since it appears to be a silent codegen issue (only visible at runtime and may be time consuming to debug) and a regression from release 2024.11.1.
Starting after #5243 landed, the following slang code, compiled to spirv, causes nvidia's driver (565.90) to segfault and mesa to reject the graphics pipeline entirely (mesa 24). Concerningly, spirv-val and the validation layers are silent. Before that PR landed, the code compiled and ran successfully.
Broken Spirv
When compiled with the affected ac6f04c1 with
...
I get the following spirv:Previous, Working Spirv
When compiled with bea1394 with the same flags, I get:
Diff
With the diff here:
I don't have a reproducer that is easy to hand over, but this pipeline is simple enough that it should still fail if thrown into a vulkan example somewhere.
With spirv-val being silent, once we figure out what's going on here, I'll file an issue there to make sure they catch the problem going forward.
This seems weird, as OpTypeStruct probably always needs to take arguments? Just a guess though.