Spirv output fails with [VUID-StandaloneSpirv-Flat-06202] (wgpu water example)

[nical]

Originally filed as #2032
I'm filing a new issue since a fix for the first part of that issue doesn't appear to fix this part.

$ cd ../../examples/water/
$ naga water.wgsl water.spv
$ spirv-val --target-env vulkan1.0 water.spv 
error: line 139: [VUID-StandaloneSpirv-Flat-06202] OpEntryPoint interfaces variable must not be vertex execution model with an input storage class for Entry Point id 479.
  %465 = OpVariable %_ptr_Input_v2int Input

[nical]

Reduced the failing shader down to:

@vertex
fn vs_main(
    @location(0) position: vec2<i32>,
) -> @builtin(position) vec4<f32> {
    return vec4<f32>(0.0, 0.0, 0.0, 1.0);
}

$ naga water.wgsl water.spv && spirv-val --target-env vulkan1.0 water.spv
error: line 16: [VUID-StandaloneSpirv-Flat-06202] OpEntryPoint interfaces variable must not be vertex execution model with an input storage class for Entry Point id 15.
  %10 = OpVariable %_ptr_Input_v2int Input

$ spirv-dis water.spv
; SPIR-V
; Version: 1.0
; Generator: Khronos; 28
; Bound: 25
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %15 "vs_main" %10 %gl_Position
               OpDecorate %10 Location 0
               OpDecorate %10 Flat
               OpDecorate %gl_Position BuiltIn Position
       %void = OpTypeVoid
      %float = OpTypeFloat 32
    %float_0 = OpConstant %float 0
    %float_1 = OpConstant %float 1
        %int = OpTypeInt 32 1
      %v2int = OpTypeVector %int 2
    %v4float = OpTypeVector %float 4
%_ptr_Input_v2int = OpTypePointer Input %v2int
         %10 = OpVariable %_ptr_Input_v2int Input
%_ptr_Output_v4float = OpTypePointer Output %v4float
%gl_Position = OpVariable %_ptr_Output_v4float Output
         %16 = OpTypeFunction %void
%_ptr_Output_float = OpTypePointer Output %float
       %uint = OpTypeInt 32 0
     %uint_1 = OpConstant %uint 1
         %15 = OpFunction %void None %16
          %9 = OpLabel
         %12 = OpLoad %v2int %10
               OpBranch %17
         %17 = OpLabel
         %18 = OpCompositeConstruct %v4float %float_0 %float_0 %float_0 %float_1
               OpStore %gl_Position %18
         %22 = OpAccessChain %_ptr_Output_float %gl_Position %uint_1
         %23 = OpLoad %float %22
         %24 = OpFNegate %float %23
               OpStore %22 %24
               OpReturn
               OpFunctionEnd

[nical]

For reference I used shader-playground to compile a somewhat similar glsl shader:

#version 330
in ivec2 position;

void main() {
    gl_Position = vec4(float(position.x), 0.0, 0.0, 1.0);
}

Giving the following spirv:

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 10
; Bound: 29
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %main "main" %_ %position
               OpSource GLSL 330
               OpName %main "main"
               OpName %gl_PerVertex "gl_PerVertex"
               OpMemberName %gl_PerVertex 0 "gl_Position"
               OpMemberName %gl_PerVertex 1 "gl_PointSize"
               OpMemberName %gl_PerVertex 2 "gl_ClipDistance"
               OpName %_ ""
               OpName %position "position"
               OpMemberDecorate %gl_PerVertex 0 BuiltIn Position
               OpMemberDecorate %gl_PerVertex 1 BuiltIn PointSize
               OpMemberDecorate %gl_PerVertex 2 BuiltIn ClipDistance
               OpDecorate %gl_PerVertex Block
               OpDecorate %position Location 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
    %v4float = OpTypeVector %float 4
       %uint = OpTypeInt 32 0
     %uint_1 = OpConstant %uint 1
%_arr_float_uint_1 = OpTypeArray %float %uint_1
%gl_PerVertex = OpTypeStruct %v4float %float %_arr_float_uint_1
%_ptr_Output_gl_PerVertex = OpTypePointer Output %gl_PerVertex
          %_ = OpVariable %_ptr_Output_gl_PerVertex Output
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
      %v2int = OpTypeVector %int 2
%_ptr_Input_v2int = OpTypePointer Input %v2int
   %position = OpVariable %_ptr_Input_v2int Input
     %uint_0 = OpConstant %uint 0
%_ptr_Input_int = OpTypePointer Input %int
    %float_0 = OpConstant %float 0
    %float_1 = OpConstant %float 1
%_ptr_Output_v4float = OpTypePointer Output %v4float
       %main = OpFunction %void None %3
          %5 = OpLabel
         %21 = OpAccessChain %_ptr_Input_int %position %uint_0
         %22 = OpLoad %int %21
         %23 = OpConvertSToF %float %22
         %26 = OpCompositeConstruct %v4float %23 %float_0 %float_0 %float_1
         %28 = OpAccessChain %_ptr_Output_v4float %_ %int_0
               OpStore %28 %26
               OpReturn
               OpFunctionEnd

There is a similar structure around the declaration of the input, the first difference that stands out is that the vertex attribute is not decorated as flat.

[nical]

... and removing OpDecorate %10 Flat appears to fix the issue.

[nical]

This could be fixed either:

• (1) During parsing in the wgsl frontend by not adding setting the default interpolation mode to every function argument or struct parameter that has a binding (see apply_default_interpolation and call sites), and in particular not do it for inputs of the vertex shader.
• (2) In the spirv backend by ignoring the specified interpolation for inputs of the vertex shader.

(1) might be tricky to depending on whether non-entry-point functions are allowed to take argments with locations, because we don't know for non-entry-point functions which stage will call them.

I filed gpuweb/gpuweb#3399 to clarify.

That said, there is the same problem for vertex attribute bindings that are specified in a struct instead of directly in the parameters of the vertex shader entry point, the parser wouldn't be able to tell whether the struct refers to inputs of the vertex shader.

[teoxoy]

I think we should generally ignore interpolation on vertex inputs and fragment outputs (in the backends).

This is somewhat related to gpuweb/gpuweb#2760

[nical]

That's how I see it as well however it's not 100% clear to me that the spirv backend has enough context. In function parameters we can see what stages the function can be used in and we get to only care about entry points, however it looks like for structs we don't necessarily know what stage a struct with @location members will be used for (and whether as input or output).

[jimblandy]

EDIT: This is incorrect. Struct arguments to fragment shaders are broken out into individual variables, and the struct is reconstituted as part of the entry point's prelude.

Regarding structs: the actual language in the Vukan spec refers to "variables":

• VUID-StandaloneSpirv-Flat-06202
  The Flat, NoPerspective, Sample, and Centroid decorations must not be used on variables with the Input storage class in a vertex shader

I think this implies that if there are such decorations on struct members, even if the struct serves as the type of an Input variable, that shouldn't be a problem.

If that is correct, then fixing this is simply a matter of changing the way we emit entry point parameters.

[magcius]

Structs already have to be deconstructed into per-stage input/output variables (function parameters and structs are just pretend for SPIR-V, which uses stage variables back and forth), and when making these per-stage variables, you should know what shader stage and direction they're part of, and thus, what interpolation attributes to use for them.

[jimblandy]

In back::spv::Writer, write_varying is called from write_function, which is passed a FunctionInterface when it's generating code for an entry point. That includes the pipeline stage, which together with the class I think gives us all the information we need.

So it's simply a matter of write_varying selectively not putting decorations on the variables.