From 23c4480d8e5100cac4262af95a8c8159c6eae589 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Tue, 31 Aug 2021 17:24:09 +0200 Subject: [PATCH] Fix switch fallthrough case in some cases. --- ...ch-non-default-fallthrough-no-phi.asm.frag | 88 +++++++++++++++ ...ch-non-default-fallthrough-no-phi.asm.frag | 105 ++++++++++++++++++ spirv_glsl.cpp | 38 ++++--- 3 files changed, 214 insertions(+), 17 deletions(-) create mode 100644 reference/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag create mode 100644 shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag diff --git a/reference/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag b/reference/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag new file mode 100644 index 00000000..34fce564 --- /dev/null +++ b/reference/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag @@ -0,0 +1,88 @@ +#version 450 + +struct _4 +{ + uint _m0; + int _m1; +}; + +struct _5 +{ + int _m0; + int _m1; +}; + +layout(location = 0) flat in int _2; +layout(location = 0) out int _3; + +_4 _16; +int _21; + +void main() +{ + bool _25 = false; + do + { + _5 _26; + _26._m0 = 0; + _26._m1 = 10; + _4 _35; + _35 = _16; + int _39; + _4 _36; + bool _59; + int _38 = 0; + for (;;) + { + if (_26._m0 < _26._m1) + { + int _27 = _26._m0; + int _28 = _26._m0 + int(1u); + _26._m0 = _28; + _36 = _4(1u, _27); + } + else + { + _4 _48 = _35; + _48._m0 = 0u; + _36 = _48; + } + bool _45_ladder_break = false; + switch (int(_36._m0)) + { + case 0: + { + _3 = _38; + _25 = true; + _59 = true; + _45_ladder_break = true; + break; + } + default: + { + _59 = false; + _45_ladder_break = true; + break; + } + case 1: + { + break; + } + } + if (_45_ladder_break) + { + break; + } + _39 = _38 + _2; + _35 = _36; + _38 = _39; + continue; + } + if (_59) + { + break; + } + break; + } while(false); +} + diff --git a/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag b/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag new file mode 100644 index 00000000..dd9a5a97 --- /dev/null +++ b/shaders-no-opt/asm/frag/switch-non-default-fallthrough-no-phi.asm.frag @@ -0,0 +1,105 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google rspirv; 0 +; Bound: 80 +; Schema: 0 + OpCapability Shader + OpCapability VulkanMemoryModel + OpExtension "SPV_KHR_vulkan_memory_model" + OpMemoryModel Logical Vulkan + OpEntryPoint Fragment %1 "main" %2 %3 + OpExecutionMode %1 OriginUpperLeft + OpMemberDecorate %_struct_14 0 Offset 0 + OpMemberDecorate %_struct_14 1 Offset 4 + OpMemberDecorate %_struct_15 0 Offset 0 + OpMemberDecorate %_struct_15 1 Offset 4 + OpDecorate %2 Location 0 + OpDecorate %3 Location 0 + OpDecorate %2 Flat + %int = OpTypeInt 32 1 + %uint = OpTypeInt 32 0 + %bool = OpTypeBool +%_ptr_Input_int = OpTypePointer Input %int +%_ptr_Output_int = OpTypePointer Output %int +%_ptr_Function_int = OpTypePointer Function %int + %void = OpTypeVoid + %_struct_14 = OpTypeStruct %uint %int + %_struct_15 = OpTypeStruct %int %int +%_ptr_Function__struct_15 = OpTypePointer Function %_struct_15 + %24 = OpTypeFunction %void + %2 = OpVariable %_ptr_Input_int Input + %3 = OpVariable %_ptr_Output_int Output + %uint_1 = OpConstant %uint 1 + %26 = OpUndef %_struct_14 + %uint_0 = OpConstant %uint 0 + %int_0 = OpConstant %int 0 + %int_10 = OpConstant %int 10 + %true = OpConstantTrue %bool + %31 = OpUndef %int + %false = OpConstantFalse %bool +%_ptr_Function_bool = OpTypePointer Function %bool + %1 = OpFunction %void None %24 + %32 = OpLabel + %76 = OpVariable %_ptr_Function_bool Function %false + %33 = OpVariable %_ptr_Function__struct_15 Function + %34 = OpVariable %_ptr_Function_int Function + %35 = OpVariable %_ptr_Function_int Function + OpSelectionMerge %72 None + OpSwitch %uint_0 %73 + %73 = OpLabel + %36 = OpLoad %int %2 + %37 = OpAccessChain %_ptr_Function_int %33 %uint_0 + OpStore %37 %int_0 + %38 = OpAccessChain %_ptr_Function_int %33 %uint_1 + OpStore %38 %int_10 + OpBranch %40 + %40 = OpLabel + %41 = OpPhi %_struct_14 %26 %73 %42 %43 + %44 = OpPhi %int %int_0 %73 %45 %43 + OpLoopMerge %48 %43 None + OpBranch %49 + %49 = OpLabel + %52 = OpLoad %int %37 + %53 = OpLoad %int %38 + %54 = OpSLessThan %bool %52 %53 + OpSelectionMerge %55 None + OpBranchConditional %54 %56 %57 + %57 = OpLabel + %65 = OpCompositeInsert %_struct_14 %uint_0 %41 0 + OpBranch %55 + %56 = OpLabel + %59 = OpLoad %int %37 + %60 = OpBitcast %int %uint_1 + %61 = OpIAdd %int %59 %60 + OpCopyMemory %34 %37 + %63 = OpLoad %int %34 + OpStore %35 %61 + OpCopyMemory %37 %35 + %64 = OpCompositeConstruct %_struct_14 %uint_1 %63 + OpBranch %55 + %55 = OpLabel + %42 = OpPhi %_struct_14 %64 %56 %65 %57 + %66 = OpCompositeExtract %uint %42 0 + %67 = OpBitcast %int %66 + OpSelectionMerge %71 None + OpSwitch %67 %69 0 %70 1 %71 + %71 = OpLabel + %45 = OpIAdd %int %44 %36 + OpBranch %43 + %70 = OpLabel + OpStore %3 %44 + OpStore %76 %true + OpBranch %48 + %69 = OpLabel + OpBranch %48 + %43 = OpLabel + OpBranch %40 + %48 = OpLabel + %79 = OpPhi %bool %false %69 %true %70 + OpSelectionMerge %77 None + OpBranchConditional %79 %72 %77 + %77 = OpLabel + OpBranch %72 + %72 = OpLabel + OpReturn + OpFunctionEnd diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index c5550ee5..472dd887 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -14974,26 +14974,30 @@ void CompilerGLSL::emit_block_chain(SPIRBlock &block) } // Might still have to flush phi variables if we branch from loop header directly to merge target. - if (flush_phi_required(block.self, block.next_block)) + // This is supposed to emit all cases where we branch from header to merge block directly. + // There are two main scenarios where cannot rely on default fallthrough. + // - There is an explicit default: label already. + // In this case, literals_to_merge need to form their own "default" case, so that we avoid executing that block. + // - Header -> Merge requires flushing PHI. In this case, we need to collect all cases and flush PHI there. + bool header_merge_requires_phi = flush_phi_required(block.self, block.next_block); + bool need_fallthrough_block = block.default_block == block.next_block || !literals_to_merge.empty(); + if ((header_merge_requires_phi && need_fallthrough_block) || !literals_to_merge.empty()) { - if (block.default_block == block.next_block || !literals_to_merge.empty()) + for (auto &case_literal : literals_to_merge) + statement("case ", to_case_label(case_literal, unsigned_case), label_suffix, ":"); + + if (block.default_block == block.next_block) { - for (auto &case_literal : literals_to_merge) - statement("case ", to_case_label(case_literal, unsigned_case), label_suffix, ":"); - - if (block.default_block == block.next_block) - { - if (is_legacy_es()) - statement("else"); - else - statement("default:"); - } - - begin_scope(); - flush_phi(block.self, block.next_block); - statement("break;"); - end_scope(); + if (is_legacy_es()) + statement("else"); + else + statement("default:"); } + + begin_scope(); + flush_phi(block.self, block.next_block); + statement("break;"); + end_scope(); } if (degenerate_switch && !is_legacy_es())