[spirv-val] OpCooperativeMatrixLoadKHR not checking if loading through an array

[spencer-lunarg]

The spec says

but found that if I remove the array, there is no validation error

@jeffbolznv so what is the exact restriction here? I notice a basic glslang shader will "loads through" the array as its in the middle of the access chain, but what was the real thing you are trying to avoid? something around variable pointer?

  %5 = OpTypeRuntimeArray %9
  %6 = OpTypeStruct %5
%15 = OpTypePointer StorageBuffer %6
  %4 = OpVariable %15 StorageBuffer

 %24 = OpAccessChain %19 %4 0 0
 %25 = OpCooperativeMatrixLoadKHR %13 %24 ...

---

TEST_F(ValidateMemory, CoopMatMatrixLoadNonArray) {
  const std::string body =
      R"(
               OpCapability Shader
               OpCapability VulkanMemoryModel
               OpCapability CooperativeMatrixKHR
               OpExtension "SPV_KHR_cooperative_matrix"
               OpMemoryModel Logical Vulkan
               OpEntryPoint GLCompute %main "main" %matA %inputA
               OpExecutionMode %main LocalSize 16 1 1
               OpDecorate %_arr_uint_uint_1 ArrayStride 4
               OpDecorate %InputA Block
               OpMemberDecorate %InputA 0 Offset 0
               OpDecorate %inputA Binding 0
               OpDecorate %inputA DescriptorSet 0
       %void = OpTypeVoid
          %4 = OpTypeFunction %void
       %uint = OpTypeInt 32 0
     %uint_3 = OpConstant %uint 3
    %uint_16 = OpConstant %uint 16
     %uint_0 = OpConstant %uint 0
         %11 = OpTypeCooperativeMatrixKHR %uint %uint_3 %uint_16 %uint_16 %uint_0
%_ptr_Private_11 = OpTypePointer Private %11
       %matA = OpVariable %_ptr_Private_11 Private
     %uint_1 = OpConstant %uint 1
%_arr_uint_uint_1 = OpTypeArray %uint %uint_1
     %InputA = OpTypeStruct %uint
%_ptr_StorageBuffer_InputA = OpTypePointer StorageBuffer %InputA
     %inputA = OpVariable %_ptr_StorageBuffer_InputA StorageBuffer
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
     %uint_5 = OpConstant %uint 5
%_ptr_StorageBuffer_uint = OpTypePointer StorageBuffer %_arr_uint_uint_1
     %v3uint = OpTypeVector %uint 3
         %26 = OpConstantComposite %v3uint %uint_16 %uint_1 %uint_1
       %main = OpFunction %void None %4
          %6 = OpLabel
         %23 = OpAccessChain %_ptr_StorageBuffer_uint %inputA %int_0
         %24 = OpCooperativeMatrixLoadKHR %11 %23 %int_0 %uint_16 MakePointerVisible|NonPrivatePointer %uint_5
               OpStore %matA %24
               OpReturn
               OpFunctionEnd
)";

  CompileSuccessfully(body.c_str(), SPV_ENV_VULKAN_1_3);
  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_3));
  EXPECT_THAT(getDiagnosticString(), HasSubstr("TODO"));
}

[jeffbolznv]

The intent is to have the instruction load several values from within a single array, rather than having it do something weird like load across several values in the structure. Though I'm not sure I've fully understood what you're asking.

[spencer-lunarg]

Lets take a basic shader (https://godbolt.org/z/EnsvTj8GK)

  %5 = OpTypeRuntimeArray %9
  %6 = OpTypeStruct %5
%15 = OpTypePointer StorageBuffer %6
  %4 = OpVariable %15 StorageBuffer
%24 = OpAccessChain %19 %4 0 0

here the access chain starts at a where index 0 goes into the OpTypeStruct then index 1 is inside the OpTypeRuntimeArray

so what I don't expect you allow is something like

struct AnotherStruct {
    uint a;
    uint b;
};
layout(set=0, binding=0) buffer InputA { AnotherStruct x[2]; } inputA;

coopMatLoad(matA, inputA.x[0].a, 0, 16, gl_CooperativeMatrixLayoutRowMajor);

which "technically" is allowed by this wording, because it when through an array... really it seems like you ment "the last item of the access chain must be an array" (there is already validation the final type accessed is a scalar/vector)

[jeffbolznv]

Yeah, for KHR_coopmat I think we meant "the last item of the access chain must be an array of scalar/vector" as you say. Be warned though that for NV_coopmat2 we intentionally allow the pointer to be into an array of any type.

[spencer-lunarg]

Be warned though that for NV_coopmat2 we intentionally allow the pointer to be into an array of any type

What bans this in CoopMat1 is the

Its type must be an OpTypePointer whose Type operand is a scalar or vector type. 

part as that is the final element of the access

I think we ment

cool... will try and get a spec change and some spirv-val added

[spencer-lunarg]

another one I found, if you generated code that looked like

layout(set=0, binding=0) coherent buffer SSBO { 
    uint8_t payload[128][128]; 
};
coopMatLoad(mat, payload, 0, 16, gl_CooperativeMatrixLayoutRowMajor);

and are you allowed to load through a double array?

[jeffbolznv]

Not 100% sure, but I think this would be invalid and if you have a 128x128 tensor you should use payload[128*128] (or better, payload[])