KhronosGroup / SPIRV-Tools

Apache License 2.0
1.09k stars 559 forks source link

Incorrect optimization of Phi nodes in SPIR-V #5815

Open mitdemverkaufer opened 2 months ago

mitdemverkaufer commented 2 months ago

The spirv-opt misoptimizes a series of Phi nodes and causes incorrect constant folding results.

In the following shader program:

#version 310 es

precision highp float;
precision highp int;

layout(location = 0) out vec4 color;

layout(binding = 0) uniform Uniform {
    int input1; // input1 = 1
};

bool b() {
  uint c = 0u;
  return bool(c);
}
bool d(int, int) {
  uint e = 0u;
  return bool(e);
}

void main() {
  int c = 0;
  int g = 0;
  int k = 0;
  for (int i = 0; i < input1; i++)   // input1 = 1, iterate only once
    if (b()) {    // b() is false
      if (d(int(gl_FragCoord), int(gl_FragCoord)))
        continue;
      break;
    } else {
      k = c;      // k = 0
      for (int j = 0; j < 2; j++)
        g = 1;    // g = 1
      k = g | k;  // k = 1
    }
  color = vec4(vec3(k), 1.0); // color = (1, 1, 1, 1)
}

The input1 is set to 1 during runtime. The program contains a series of control-flow structures. The outer loop should only iterate once since input = 1. The first if-statement should evaluate to false, thus the else branch is executed. The variable k is set to be 1 in the statement k = g | k since g is 1. As such, the final color should be white.

However, the spirv-opt misoptimizes the shader and the final color is black.

Steps to reproduce

I provide a minimal Vulkan application that runs the give shader program. Simply follow the instruction in the README.md in the package. It should take less than a minute to reproduce the issue. I also provide the non-optimized, optimized, and the disassembled code from the optimized SPIR-V binary in the package.

s-perron commented 2 months ago

The problem is the ssa-rewrite pass. It is an odd error with a very particular control flow.

This is the reduced spir-v, and it can be reproduced by running the ssa-rewrite pass:

Command:

spirv-opt t.spv -o o.spv --ssa-rewrite

Input:

               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main"
               OpExecutionMode %main OriginUpperLeft
               OpSource ESSL 310
               OpName %main "main"
               OpName %k "k"
               OpName %i "i"
               OpName %j "j"
       %void = OpTypeVoid
          %6 = OpTypeFunction %void
       %bool = OpTypeBool
      %false = OpConstantFalse %bool
        %int = OpTypeInt 32 1
%_ptr_Function_int = OpTypePointer Function %int
      %int_0 = OpConstant %int 0
      %float = OpTypeFloat 32
      %int_1 = OpConstant %int 1
       %main = OpFunction %void None %6
         %14 = OpLabel
          %k = OpVariable %_ptr_Function_int Function
          %i = OpVariable %_ptr_Function_int Function
          %j = OpVariable %_ptr_Function_int Function
               OpStore %k %int_0
               OpStore %i %int_0
               OpBranch %15
         %15 = OpLabel
               OpLoopMerge %16 %17 None
               OpBranch %18
         %18 = OpLabel
         %19 = OpLoad %int %i
         %20 = OpSLessThan %bool %19 %int_1
               OpBranchConditional %20 %21 %16
         %21 = OpLabel
               OpSelectionMerge %22 None
               OpBranchConditional %false %23 %24
         %23 = OpLabel
               OpBranchConditional %false %17 %16
         %24 = OpLabel
               OpStore %k %int_0
               OpStore %j %int_0
               OpBranch %25
         %25 = OpLabel
         %26 = OpLoad %int %j
         %27 = OpSLessThan %bool %26 %int_1
               OpLoopMerge %28 %29 None
               OpBranchConditional %27 %29 %28
         %29 = OpLabel
         %30 = OpLoad %int %j
         %31 = OpIAdd %int %30 %int_1
               OpStore %j %31
               OpBranch %25
         %28 = OpLabel
          ; If this load is removed, the result is correct.
         %32 = OpLoad %int %k
               OpStore %k %int_1
               OpBranch %22
         %22 = OpLabel
               OpBranch %17
         %17 = OpLabel
         %33 = OpLoad %int %i
         %34 = OpIAdd %int %33 %int_1
               OpStore %i %34
               OpBranch %15
         %16 = OpLabel
         %35 = OpLoad %int %k
         %36 = OpConvertSToF %float %35
               OpReturn
               OpFunctionEnd

Output:

%41 = OpPhi %int %int_0 %14 %44 %17 ; At the start of the outer loop. This is correct.
...
; In the continue for the loop. The %int_0 is incorrect, It should be %int_1 when coming 
; from block %22.
%44 = OpPhi %int %41 %23 %int_0 %22 
...
%36 = OpConvertSToF %float %41