Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RuntimeArray within a RuntimeArray generated from HLSL #2439

Closed
ruiling opened this issue Oct 28, 2020 · 6 comments · Fixed by KhronosGroup/SPIRV-Tools#5094 or #3130
Closed

RuntimeArray within a RuntimeArray generated from HLSL #2439

ruiling opened this issue Oct 28, 2020 · 6 comments · Fixed by KhronosGroup/SPIRV-Tools#5094 or #3130
Assignees

Comments

@ruiling
Copy link

ruiling commented Oct 28, 2020

I am investigating an issue when running vulkancts.
https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/tessellation/vktTessellationFractionalSpacingTests.cpp#L498
The tessellation evaluation shader is written in hlsl:

struct OutputStruct
{
    int numInvocations;
    float tessCoord[];
};
globallycoherent RWStructuredBuffer <OutputStruct> Output : register(b1);

void main(float2 tessCoords : SV_DOMAINLOCATION)
{
    int index;
    InterlockedAdd(Output[0].numInvocations, 1, index);
    Output[0].tessCoord[index] = tessCoords.x;
}

I compiled with ./glslangValidator -e main -V 1.tese.hlsl -o 1.spv; spirv-dis --raw-id 1.spv -o -
The compiled spv is below:

               OpCapability Tessellation
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint TessellationEvaluation %4 "main" %39
               OpSource HLSL 500
               OpName %4 "main"
               OpName %11 "@main(vf2;"
               OpName %10 "tessCoords"
               OpName %15 "index"
               OpName %17 "OutputStruct"
               OpMemberName %17 0 "numInvocations"
               OpMemberName %17 1 "tessCoord"
               OpName %19 "Output"
               OpMemberName %19 0 "@data"
               OpName %21 "Output"
               OpName %36 "tessCoords"
               OpName %39 "tessCoords"
               OpName %44 "param"
               OpDecorate %16 ArrayStride 4
               OpMemberDecorate %17 0 Offset 0
               OpMemberDecorate %17 1 Offset 4
               OpDecorate %18 ArrayStride 8
               OpMemberDecorate %19 0 Coherent
               OpMemberDecorate %19 0 Offset 0
               OpDecorate %19 BufferBlock
               OpDecorate %21 DescriptorSet 0
               OpDecorate %21 Binding 1
               OpDecorate %39 Patch
               OpDecorate %39 BuiltIn TessCoord
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeFloat 32
          %7 = OpTypeVector %6 2
          %8 = OpTypePointer Function %7
          %9 = OpTypeFunction %2 %8
         %13 = OpTypeInt 32 1
         %14 = OpTypePointer Function %13
         %16 = OpTypeRuntimeArray %6
         %17 = OpTypeStruct %13 %16
         %18 = OpTypeRuntimeArray %17
         %19 = OpTypeStruct %18
         %20 = OpTypePointer Uniform %19
         %21 = OpVariable %20 Uniform
         %22 = OpConstant %13 0
         %23 = OpTypePointer Uniform %13
         %25 = OpConstant %13 1
         %26 = OpTypeInt 32 0
         %27 = OpConstant %26 1
         %28 = OpConstant %26 0
         %31 = OpTypePointer Function %6
         %34 = OpTypePointer Uniform %6
         %37 = OpTypeVector %6 3
         %38 = OpTypePointer Input %37
         %39 = OpVariable %38 Input
          %4 = OpFunction %2 None %3
          %5 = OpLabel
         %36 = OpVariable %8 Function
         %44 = OpVariable %8 Function
         %40 = OpLoad %37 %39
         %41 = OpCompositeExtract %6 %40 0
         %42 = OpCompositeExtract %6 %40 1
         %43 = OpCompositeConstruct %7 %41 %42
               OpStore %36 %43
         %45 = OpLoad %7 %36
               OpStore %44 %45
         %46 = OpFunctionCall %2 %11 %44
               OpReturn
               OpFunctionEnd
         %11 = OpFunction %2 None %9
         %10 = OpFunctionParameter %8
         %12 = OpLabel
         %15 = OpVariable %14 Function
         %24 = OpAccessChain %23 %21 %22 %22 %22
         %29 = OpAtomicIAdd %13 %24 %27 %28 %25
               OpStore %15 %29
         %30 = OpLoad %13 %15
         %32 = OpAccessChain %31 %10 %28
         %33 = OpLoad %6 %32
         %35 = OpAccessChain %34 %21 %22 %22 %25 %30
               OpStore %35 %33
               OpReturn
               OpFunctionEnd

The problematic piece is:

         %16 = OpTypeRuntimeArray %6
         %17 = OpTypeStruct %13 %16
         %18 = OpTypeRuntimeArray %17

This means that the RWStructuredBuffer Output was translated as OpTypeRuntimeArray, as tessCoord is also translated as OpTypeRuntimeArray. This causes a situation that a run-time array was included in another run-time array, on which our compiler has some issue. I am not sure is this expected behaviour? In fact, the Output is only referenced by Output[0], so I think we can just treat it as an array with length 1?

@ruiling
Copy link
Author

ruiling commented Nov 1, 2020

@greg-lunarg Do you have any comments on this?

@ruiling
Copy link
Author

ruiling commented Jan 21, 2021

ping. anybody take a look?

@greg-lunarg
Copy link
Contributor

Hi. Sorry missed your first ping. Yes, I will take a look.

@ruiling
Copy link
Author

ruiling commented Mar 2, 2021

ping

@greg-lunarg
Copy link
Contributor

Very sorry for the delayed response.

This shader is not a valid Vulkan shader. It violates the following rule from the Vulkan spec:

OpTypeRuntimeArray must only be used for the last member of an OpTypeStruct that is in the
StorageBuffer or PhysicalStorageBuffer storage class decorated as Block, or that is in the Uniform
storage class decorated as BufferBlock.

The OutputStruct type (%17) violates this rule ie. it contains a runtime array but it does not have a BufferBlock decoration. When compiling for Vulkan, this should raise an error from glslang.

So this is a bug with glslang and should be fixed ie. an error message needs to be generated when compiling for Vulkan.

Additionally, the CTS test is invalid and should have an issue raised. In addition, spirv-val does not catch this condition and should have an issue raised.

Final note to developer: when compiling for Vulkan1.2 (post-SPIRV1.3), should use Block decoration on Output type (%19) instead of BufferBlock which is deprecated after SPIRV1.3.

@greg-lunarg
Copy link
Contributor

Note on last note: SPIRV1.4 can be used with Vulkan1.1 and 1.0, so the SPIR-V version (> 1.3) should be the only determinant on using the Block decoration.

FoggyMist pushed a commit to FoggyMist/SPIRV-Tools that referenced this issue Feb 1, 2023
Partially fixes KhronosGroup/glslang#2439

* When OpTypeStruct is used in Vulkan env and its last member
  is a RuntimeArray, check if the struct is decorated with
  Block or BufferBlock, as required by VUID-...-04680.
FoggyMist added a commit to FoggyMist/SPIRV-Tools that referenced this issue Feb 1, 2023
Partially fixes KhronosGroup/glslang#2439

* When OpTypeStruct is used in Vulkan env and its last member
  is a RuntimeArray, check if the struct is decorated with
  Block or BufferBlock, as required by VUID-...-04680.
FoggyMist added a commit to FoggyMist/SPIRV-Tools that referenced this issue Feb 1, 2023
Partially fixes KhronosGroup/glslang#2439

* When OpTypeStruct is used in Vulkan env and its last member
  is a RuntimeArray, check if the struct is decorated with
  Block or BufferBlock, as required by VUID-...-04680.
FoggyMist added a commit to FoggyMist/glslang that referenced this issue Feb 1, 2023
Fixes KhronosGroup#2439

When decorating a struct for Vulkan, add [Buffer]Block decoration if the
struct has a RuntimeArray member, as required by VUID-...-04680.
arcady-lunarg pushed a commit that referenced this issue Feb 2, 2023
Fixes #2439

When decorating a struct for Vulkan, add [Buffer]Block decoration if the
struct has a RuntimeArray member, as required by VUID-...-04680.
alan-baker pushed a commit to KhronosGroup/SPIRV-Tools that referenced this issue Feb 3, 2023
Contributes to KhronosGroup/glslang#2439

* When OpTypeStruct is used in Vulkan env and its last member
  is a RuntimeArray, check if the struct is decorated with
  Block or BufferBlock, as required by VUID-...-04680.
kd-11 pushed a commit to kd-11/glslang that referenced this issue Dec 11, 2023
Fixes KhronosGroup#2439

When decorating a struct for Vulkan, add [Buffer]Block decoration if the
struct has a RuntimeArray member, as required by VUID-...-04680.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants