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

spirv-val: No validation against non-uniform derivative instructions #5455

Closed
LLJJDD opened this issue Oct 27, 2023 · 4 comments
Closed

spirv-val: No validation against non-uniform derivative instructions #5455

LLJJDD opened this issue Oct 27, 2023 · 4 comments
Assignees

Comments

@LLJJDD
Copy link
Contributor

LLJJDD commented Oct 27, 2023

According to https://registry.khronos.org/vulkan/specs/1.3/html/vkspec.html#shaders-derivative-operations:
All dynamic instances of explicit derivative instructions (OpDPdx*, OpDPdy*, and OpFwidth*) must be executed in control flow that is uniform within a derivative group.

The following SPIR-V violates the rule, but spirv-val outputs nothing:

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %condition %value
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 450
               OpName %main "main"
               OpName %condition "condition"
               OpName %test "test"
               OpName %value "value"
               OpDecorate %condition Location 0
               OpDecorate %value Location 1
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Input_float = OpTypePointer Input %float
  %condition = OpVariable %_ptr_Input_float Input
    %float_1 = OpConstant %float 1
       %bool = OpTypeBool
%_ptr_Function_float = OpTypePointer Function %float
      %value = OpVariable %_ptr_Input_float Input
       %main = OpFunction %void None %3
          %5 = OpLabel
       %test = OpVariable %_ptr_Function_float Function
          %9 = OpLoad %float %condition
         %12 = OpFOrdGreaterThan %bool %9 %float_1
               OpSelectionMerge %14 None
               OpBranchConditional %12 %13 %14
         %13 = OpLabel
               OpKill
         %14 = OpLabel
         %19 = OpLoad %float %value
         %20 = OpDPdx %float %19
               OpStore %test %20
               OpReturn
               OpFunctionEnd
@danginsburg
Copy link
Contributor

As a bit of context, we hit a bug with the use of OpKill, which DXC generates for clip and is also still in use if compiling for SPIR-V before v1.6 or without VK_EXT_shader_demote_to_helper_invocation. It would be great if spirv-val were able to catch this case (and better yet if the error could point users towards using OpDemoteToHelperInvocation and say that OpKill is deprecated).

Related: microsoft/DirectXShaderCompiler#5937

@s-perron
Copy link
Collaborator

s-perron commented Nov 1, 2023

I'll let @alan-baker confirm, but I do not believe it is possible for spirv-val to validate this because it is a runtime condition. The validator cannot know for sure that the branch will not be quad uniform.

We wrote a tool spirv-lint to try to find some of these potential problems. Not much development has taken place, but the one thing that was done was to look for this situation and warn the user.

For the example above its output is:

» spirv-lint t.spv
warning: line 0: derivative with divergent control flow located in block %16
warning: line 0: block %16 is divergent
warning: line 0: because it depends on a conditional branch on divergent value %15
  OpBranchConditional %15 %17 %16

warning: line 0: value %15 is divergent
warning: line 0: because %15 uses value %14in its definition, which is divergent
  %15 = OpFOrdGreaterThan %bool %14 %float_1

warning: line 0: because it has a divergent definition
  %14 = OpLoad %float %condition

» spirv-dis t.spv
; SPIR-V
; Version: 1.1
; Generator: Khronos SPIR-V Tools Assembler; 0
; Bound: 20
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %condition %value
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 450
               OpName %main "main"
               OpName %condition "condition"
               OpName %test "test"
               OpName %value "value"
               OpDecorate %condition Location 0
               OpDecorate %value Location 1
       %void = OpTypeVoid
          %7 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Input_float = OpTypePointer Input %float
  %condition = OpVariable %_ptr_Input_float Input
    %float_1 = OpConstant %float 1
       %bool = OpTypeBool
%_ptr_Function_float = OpTypePointer Function %float
      %value = OpVariable %_ptr_Input_float Input
       %main = OpFunction %void None %7
         %13 = OpLabel
       %test = OpVariable %_ptr_Function_float Function
         %14 = OpLoad %float %condition
         %15 = OpFOrdGreaterThan %bool %14 %float_1
               OpSelectionMerge %16 None
               OpBranchConditional %15 %17 %16
         %17 = OpLabel
               OpKill
         %16 = OpLabel
         %18 = OpLoad %float %value
         %19 = OpDPdx %float %18
               OpStore %test %19
               OpReturn
               OpFunctionEnd

@alan-baker
Copy link
Contributor

@s-perron explained it well. It's ok for a linter to have false positives, but it's not for the validator. So the cases that can be caught might do more harm than good by leading to overconfidence.

@LLJJDD
Copy link
Contributor Author

LLJJDD commented Nov 7, 2023

Thanks! That makes sense.

@LLJJDD LLJJDD closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants