spirv-val: VUID-FragCoord-FragCoord-04212 interaction with PerVertexKHR

[Keenuts]

Hi,

I'm working on a bug in DXC related to SPV_KHR_fragment_shader_barycentric.
From what I gather, an input decorated with the PerVertexKHR becomes a an array of N elements, N being the number of vertices composing the primitive.

float4 frag(nointerpolation float4 position : SV_Position) : SV_Target {
    return GetAttributeAtVertex(position, 1);
}

Building this code yields this SPIR-V:

               OpCapability Shader
               OpCapability FragmentBarycentricKHR
               OpExtension "SPV_KHR_fragment_shader_barycentric"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %frag "frag" %gl_FragCoord %out_var_SV_Target
               OpExecutionMode %frag OriginUpperLeft
               OpSource HLSL 610
               OpName %out_var_SV_Target "out.var.SV_Target"
               OpName %frag "frag"
               OpDecorate %gl_FragCoord BuiltIn FragCoord
               OpDecorate %gl_FragCoord Flat
               OpDecorate %out_var_SV_Target Location 0
               OpDecorate %gl_FragCoord PerVertexKHR
       %uint = OpTypeInt 32 0
     %uint_3 = OpConstant %uint 3
      %float = OpTypeFloat 32
    %v4float = OpTypeVector %float 4
%_arr_v4float_uint_3 = OpTypeArray %v4float %uint_3
%_ptr_Input__arr_v4float_uint_3 = OpTypePointer Input %_arr_v4float_uint_3
%_ptr_Output_v4float = OpTypePointer Output %v4float
       %void = OpTypeVoid
         %12 = OpTypeFunction %void
%gl_FragCoord = OpVariable %_ptr_Input__arr_v4float_uint_3 Input
%out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
%_ptr_Input_v4float = OpTypePointer Input %v4float
     %uint_1 = OpConstant %uint 1
       %frag = OpFunction %void None %12
         %15 = OpLabel
         %16 = OpAccessChain %_ptr_Input_v4float %gl_FragCoord %uint_1
         %17 = OpLoad %v4float %16
               OpStore %out_var_SV_Target %17
               OpReturn
               OpFunctionEnd

So we have:
OpDecorate %gl_FragCoord BuiltIn FragCoord
OpDecorate %gl_FragCoord PerVertexKHR
%gl_FragCoord = OpVariable %_ptr_Input__arr_v4float_uint_3 Input %out_var_SV_Target = OpVariable %_ptr_Output_v4float Output

This seem to go in the direction of the extension spec, but SPIRV-val complains that we go against VUID-FragCoord-FragCoord-04212 as the variable decorated with FragCoord doesn't have the correct type.

I believe this is a "validation" error, but since not SPV_KHR_fragment_shader_barycentric nor VK_KHR_fragment_shader_barycentric relaxes the validation rules, maybe that's a spec issue?

[alan-baker]

@spencer-lunarg @gnl21 any thoughts on this. I find the Vulkan spec surrounding which IO can be arrayed to be a confusing read. It seems there should be a part of the built-in variables section saying if the variable/struct member is decorated with PerVertex then the arraying should be peeled before applying the VUs.

[gnl21]

My immediate thought is that I'm not sure that you're supposed to be able to use PerVertex on builtins at all, although the spec doesn't actually say that. GLSL doesn't allow this and the Vulkan spec only talks about matching PerVertex inputs with the user outputs of the previous stage. From an implementation point-of-view, these values are normally generated per-fragment, so it's not clear that the vertex values would be available, or possibly are even well-defined.

Having said all that, if this were to be allowed then yes, I think the array level would need to be removed first so that the value at each vertex follows the normal rules for what the value has to be (a vector of floats, in this case).

[Keenuts]

Which part of the spec limits PerVertexKHR to user inputs?

Reading:
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_fragment_shader_barycentric.html

PerVertexKHR, which indicates that a fragment shader input will not have interpolated values, but instead must be accessed with an extra array index that identifies one of the vertices of the primitive producing the fragment

Seems like any vertex shader output can be forwarded to the fragment shader input as-is, including builtins..

(On the DXC side, the feature is mostly broken, so any decision is fine for me)

[gnl21]

Which part of the spec limits PerVertexKHR to user inputs?

There isn't really one, as I said, but I think this was the intent, so the lack of a restriction would be a bug. Technically the spec does have the VUs:

The variable decorated with FragCoord must be declared as a four-component vector of
32-bit floating-point values

and

Variables with a Storage Class of Input in a fragment shader stage that are decorated with
PerVertexKHR must be declared as arrays

which would prohibit this, but if it is supposed to be banned then it should be much clearer.

Seems like any vertex shader output can be forwarded to the fragment shader input as-is, including builtins.

The problem is that some of the fragment shader builtin inputs do not have corresponding vertex shader outputs. I can imagine that FragCoord is supposed to match Position from the vertex stage, which mostly makes sense (although the viewport transform, etc come between them), but something like FrontFacing doesn't have a corresponding vertex output.

My implementation doesn't support this extension at the moment, but it would be disproportionately expensive for us to provide access to these not-quite-vertex-outputs at each vertex.

[spencer-lunarg]

I guess @gnl21 summed up my thoughts... I guess from my reading of just the spec, spirv-val is doing what it should... maybe @stu-s (being the champion of VK_KHR_fragment_shader_barycentric) might know if this is a spec oversight or intended

[Keenuts]

Right, thanks!

So to recap, PerVertexKHR shall only be used on used-defined outputs, and no builtins at all. I can work with that on the compiler side :)

[stu-s]

I guess @gnl21 summed up my thoughts... I guess from my reading of just the spec, spirv-val is doing what it should... maybe @stu-s (being the champion of VK_KHR_fragment_shader_barycentric) might know if this is a spec oversight or intended

As @gnl21 said, it's possibly missing a restriction. I don't think it was intended, though I didn't really touch this bit of the spec in the move from NV to KHR other than the "missing vertex" stuff.