From e3d4dddfec57f4a76fb52308f8277360638d06c7 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Mon, 26 Aug 2019 09:56:00 +0200 Subject: [PATCH] Fix variable scope when switch block exits multiple times. Inner scope can still dominate here, so we need to be conservative when we observe switch blocks specifically. Normal selection merges cannot merge from multiple paths. --- ...tch-single-case-multiple-exit-cfg.asm.frag | 29 ++++++++++ ...tch-single-case-multiple-exit-cfg.asm.frag | 57 +++++++++++++++++++ spirv_cfg.cpp | 25 +++++++- 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 reference/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag create mode 100644 shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag diff --git a/reference/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag b/reference/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag new file mode 100644 index 00000000..66ac130d --- /dev/null +++ b/reference/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag @@ -0,0 +1,29 @@ +#version 310 es +precision mediump float; +precision highp int; + +layout(location = 0) out highp vec4 _GLF_color; + +vec2 _19; + +void main() +{ + highp vec2 _30; + switch (0) + { + default: + { + if (gl_FragCoord.x != gl_FragCoord.x) + { + _30 = _19; + break; + } + highp vec2 _29 = _19; + _29.y = _19.y; + _30 = _29; + break; + } + } + _GLF_color = vec4(_30, 1.0, 1.0); +} + diff --git a/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag b/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag new file mode 100644 index 00000000..d2bd15a9 --- /dev/null +++ b/shaders-no-opt/asm/frag/switch-single-case-multiple-exit-cfg.asm.frag @@ -0,0 +1,57 @@ +; SPIR-V +; Version: 1.0 +; Generator: Khronos Glslang Reference Front End; 7 +; Bound: 54 +; Schema: 0 + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" %gl_FragCoord %_GLF_color + OpExecutionMode %main OriginUpperLeft + OpSource ESSL 310 + OpName %main "main" + OpName %gl_FragCoord "gl_FragCoord" + OpName %_GLF_color "_GLF_color" + OpDecorate %gl_FragCoord BuiltIn FragCoord + OpDecorate %_GLF_color Location 0 + %void = OpTypeVoid + %3 = OpTypeFunction %void + %int = OpTypeInt 32 1 + %int_0 = OpConstant %int 0 + %float = OpTypeFloat 32 + %v4float = OpTypeVector %float 4 +%_ptr_Input_v4float = OpTypePointer Input %v4float +%gl_FragCoord = OpVariable %_ptr_Input_v4float Input + %uint = OpTypeInt 32 0 + %uint_0 = OpConstant %uint 0 +%_ptr_Input_float = OpTypePointer Input %float + %bool = OpTypeBool + %v2float = OpTypeVector %float 2 +%_ptr_Output_v4float = OpTypePointer Output %v4float + %_GLF_color = OpVariable %_ptr_Output_v4float Output + %float_1 = OpConstant %float 1 + %52 = OpUndef %v2float + %main = OpFunction %void None %3 + %5 = OpLabel + OpSelectionMerge %9 None + OpSwitch %int_0 %8 + %8 = OpLabel + %17 = OpAccessChain %_ptr_Input_float %gl_FragCoord %uint_0 + %18 = OpLoad %float %17 + %22 = OpFOrdNotEqual %bool %18 %18 + OpSelectionMerge %24 None + OpBranchConditional %22 %23 %24 + %23 = OpLabel + OpBranch %9 + %24 = OpLabel + %33 = OpCompositeExtract %float %52 1 + %51 = OpCompositeInsert %v2float %33 %52 1 + OpBranch %9 + %9 = OpLabel + %53 = OpPhi %v2float %52 %23 %51 %24 + %42 = OpCompositeExtract %float %53 0 + %43 = OpCompositeExtract %float %53 1 + %48 = OpCompositeConstruct %v4float %42 %43 %float_1 %float_1 + OpStore %_GLF_color %48 + OpReturn + OpFunctionEnd diff --git a/spirv_cfg.cpp b/spirv_cfg.cpp index ed31f236..8f6f024c 100644 --- a/spirv_cfg.cpp +++ b/spirv_cfg.cpp @@ -148,14 +148,35 @@ bool CFG::post_order_visit(uint32_t block_id) // Add a fake branch so any dominator in either the if (), or else () block, or a lone case statement // will be hoisted out to outside the selection merge. // If size > 1, the variable will be automatically hoisted, so we should not mess with it. + // The exception here is switch blocks, where we can have multiple edges to merge block, + // all coming from same scope, so be more conservative in this case. // Adding fake branches unconditionally breaks parameter preservation analysis, // which looks at how variables are accessed through the CFG. auto pred_itr = preceding_edges.find(block.next_block); if (pred_itr != end(preceding_edges)) { auto &pred = pred_itr->second; - if (pred.size() == 1 && *pred.begin() != block_id) - add_branch(block_id, block.next_block); + auto succ_itr = succeeding_edges.find(block_id); + uint32_t num_succeeding_edges = 0; + if (succ_itr != end(succeeding_edges)) + num_succeeding_edges = succ_itr->second.size(); + + if (block.terminator == SPIRBlock::MultiSelect && num_succeeding_edges == 1) + { + // Multiple branches can come from the same scope due to "break;", so we need to assume that all branches + // come from same case scope in worst case, even if there are multiple preceding edges. + // If we have more than one succeeding edge from the block header, it should be impossible + // to have a dominator be inside the block. + // Only case this can go wrong is if we have 2 or more edges from block header and + // 2 or more edges to merge block, and still have dominator be inside a case label. + if (!pred.empty()) + add_branch(block_id, block.next_block); + } + else + { + if (pred.size() == 1 && *pred.begin() != block_id) + add_branch(block_id, block.next_block); + } } else {