From 54cc0b01f6972dffe686ee576fef5f6ce6373fe0 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Thu, 17 Sep 2020 12:02:43 +0200 Subject: [PATCH 1/2] Deal with case where a selection construct conditionally merges/breaks. --- ...op-body-dominator-continue-access.asm.frag | 5 + .../early-conditional-return-switch.asm.frag | 67 +++++++++ .../early-conditional-return-switch.asm.frag | 133 ++++++++++++++++++ spirv_glsl.cpp | 23 +++ 4 files changed, 228 insertions(+) create mode 100644 reference/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag create mode 100644 shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag diff --git a/reference/opt/shaders/asm/frag/loop-body-dominator-continue-access.asm.frag b/reference/opt/shaders/asm/frag/loop-body-dominator-continue-access.asm.frag index 672fd9a8b..df32f1a45 100644 --- a/reference/opt/shaders/asm/frag/loop-body-dominator-continue-access.asm.frag +++ b/reference/opt/shaders/asm/frag/loop-body-dominator-continue-access.asm.frag @@ -55,6 +55,11 @@ void main() break; } } + if (_225) + { + _228 = _229; + break; + } _228 = -1; break; } while(false); diff --git a/reference/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag b/reference/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag new file mode 100644 index 000000000..34fffb85a --- /dev/null +++ b/reference/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag @@ -0,0 +1,67 @@ +#version 450 + +layout(binding = 0, std140) uniform type_gCBuffarrayIndex +{ + uint gArrayIndex; +} gCBuffarrayIndex; + +uniform sampler2D SPIRV_Cross_Combinedg_textureArray0SPIRV_Cross_DummySampler; +uniform sampler2D SPIRV_Cross_Combinedg_textureArray1SPIRV_Cross_DummySampler; +uniform sampler2D SPIRV_Cross_Combinedg_textureArray2SPIRV_Cross_DummySampler; +uniform sampler2D SPIRV_Cross_Combinedg_textureArray3SPIRV_Cross_DummySampler; + +layout(location = 0) out vec4 out_var_SV_TARGET; + +vec4 _32; + +void main() +{ + vec4 _80; + do + { + vec4 _77; + bool _78; + switch (gCBuffarrayIndex.gArrayIndex) + { + case 0u: + { + _77 = texelFetch(SPIRV_Cross_Combinedg_textureArray0SPIRV_Cross_DummySampler, ivec3(int(gl_FragCoord.x), int(gl_FragCoord.y), 0).xy, 0); + _78 = true; + break; + } + case 1u: + { + _77 = texelFetch(SPIRV_Cross_Combinedg_textureArray1SPIRV_Cross_DummySampler, ivec3(int(gl_FragCoord.x), int(gl_FragCoord.y), 0).xy, 0); + _78 = true; + break; + } + case 2u: + { + _77 = texelFetch(SPIRV_Cross_Combinedg_textureArray2SPIRV_Cross_DummySampler, ivec3(int(gl_FragCoord.x), int(gl_FragCoord.y), 0).xy, 0); + _78 = true; + break; + } + case 3u: + { + _77 = texelFetch(SPIRV_Cross_Combinedg_textureArray3SPIRV_Cross_DummySampler, ivec3(int(gl_FragCoord.x), int(gl_FragCoord.y), 0).xy, 0); + _78 = true; + break; + } + default: + { + _77 = _32; + _78 = false; + break; + } + } + if (_78) + { + _80 = _77; + break; + } + _80 = vec4(0.0, 1.0, 0.0, 1.0); + break; + } while(false); + out_var_SV_TARGET = _80; +} + diff --git a/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag b/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag new file mode 100644 index 000000000..d789ce36b --- /dev/null +++ b/shaders-no-opt/asm/frag/early-conditional-return-switch.asm.frag @@ -0,0 +1,133 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google spiregg; 0 +; Bound: 81 +; Schema: 0 + OpCapability Shader + OpCapability Sampled1D + OpCapability Image1D + OpCapability SampledBuffer + OpCapability ImageBuffer + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %PsTextureLoadArray "main" %gl_FragCoord %out_var_SV_TARGET + OpExecutionMode %PsTextureLoadArray OriginUpperLeft + OpSource HLSL 500 + OpName %type_2d_image "type.2d.image" + OpName %type_gCBuffarrayIndex "type.gCBuffarrayIndex" + OpMemberName %type_gCBuffarrayIndex 0 "gArrayIndex" + OpName %gCBuffarrayIndex "gCBuffarrayIndex" + OpName %g_textureArray0 "g_textureArray0" + OpName %g_textureArray1 "g_textureArray1" + OpName %g_textureArray2 "g_textureArray2" + OpName %g_textureArray3 "g_textureArray3" + OpName %out_var_SV_TARGET "out.var.SV_TARGET" + OpName %PsTextureLoadArray "PsTextureLoadArray" + OpDecorate %gl_FragCoord BuiltIn FragCoord + OpDecorate %out_var_SV_TARGET Location 0 + OpDecorate %gCBuffarrayIndex DescriptorSet 0 + OpDecorate %gCBuffarrayIndex Binding 0 + OpDecorate %g_textureArray0 DescriptorSet 0 + OpDecorate %g_textureArray0 Binding 0 + OpDecorate %g_textureArray1 DescriptorSet 0 + OpDecorate %g_textureArray1 Binding 1 + OpDecorate %g_textureArray2 DescriptorSet 0 + OpDecorate %g_textureArray2 Binding 2 + OpDecorate %g_textureArray3 DescriptorSet 0 + OpDecorate %g_textureArray3 Binding 3 + OpMemberDecorate %type_gCBuffarrayIndex 0 Offset 0 + OpDecorate %type_gCBuffarrayIndex Block + %uint = OpTypeInt 32 0 + %int = OpTypeInt 32 1 + %int_0 = OpConstant %int 0 + %float = OpTypeFloat 32 + %float_0 = OpConstant %float 0 + %float_1 = OpConstant %float 1 + %v4float = OpTypeVector %float 4 + %18 = OpConstantComposite %v4float %float_0 %float_1 %float_0 %float_1 +%type_2d_image = OpTypeImage %float 2D 2 0 0 1 Unknown +%_ptr_UniformConstant_type_2d_image = OpTypePointer UniformConstant %type_2d_image +%type_gCBuffarrayIndex = OpTypeStruct %uint +%_ptr_Uniform_type_gCBuffarrayIndex = OpTypePointer Uniform %type_gCBuffarrayIndex +%_ptr_Input_v4float = OpTypePointer Input %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float + %void = OpTypeVoid + %24 = OpTypeFunction %void +%_ptr_Uniform_uint = OpTypePointer Uniform %uint + %v3int = OpTypeVector %int 3 + %v2int = OpTypeVector %int 2 +%gCBuffarrayIndex = OpVariable %_ptr_Uniform_type_gCBuffarrayIndex Uniform +%g_textureArray0 = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant +%g_textureArray1 = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant +%g_textureArray2 = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant +%g_textureArray3 = OpVariable %_ptr_UniformConstant_type_2d_image UniformConstant +%gl_FragCoord = OpVariable %_ptr_Input_v4float Input +%out_var_SV_TARGET = OpVariable %_ptr_Output_v4float Output + %uint_0 = OpConstant %uint 0 + %bool = OpTypeBool + %false = OpConstantFalse %bool + %true = OpConstantTrue %bool + %32 = OpUndef %v4float +%PsTextureLoadArray = OpFunction %void None %24 + %33 = OpLabel + %34 = OpLoad %v4float %gl_FragCoord + OpSelectionMerge %35 None + OpSwitch %uint_0 %36 + %36 = OpLabel + %37 = OpAccessChain %_ptr_Uniform_uint %gCBuffarrayIndex %int_0 + %38 = OpLoad %uint %37 + OpSelectionMerge %39 None + OpSwitch %38 %40 0 %41 1 %42 2 %43 3 %44 + %41 = OpLabel + %45 = OpCompositeExtract %float %34 0 + %46 = OpCompositeExtract %float %34 1 + %47 = OpConvertFToS %int %45 + %48 = OpConvertFToS %int %46 + %49 = OpCompositeConstruct %v3int %47 %48 %int_0 + %50 = OpVectorShuffle %v2int %49 %49 0 1 + %51 = OpLoad %type_2d_image %g_textureArray0 + %52 = OpImageFetch %v4float %51 %50 Lod %int_0 + OpBranch %39 + %42 = OpLabel + %53 = OpCompositeExtract %float %34 0 + %54 = OpCompositeExtract %float %34 1 + %55 = OpConvertFToS %int %53 + %56 = OpConvertFToS %int %54 + %57 = OpCompositeConstruct %v3int %55 %56 %int_0 + %58 = OpVectorShuffle %v2int %57 %57 0 1 + %59 = OpLoad %type_2d_image %g_textureArray1 + %60 = OpImageFetch %v4float %59 %58 Lod %int_0 + OpBranch %39 + %43 = OpLabel + %61 = OpCompositeExtract %float %34 0 + %62 = OpCompositeExtract %float %34 1 + %63 = OpConvertFToS %int %61 + %64 = OpConvertFToS %int %62 + %65 = OpCompositeConstruct %v3int %63 %64 %int_0 + %66 = OpVectorShuffle %v2int %65 %65 0 1 + %67 = OpLoad %type_2d_image %g_textureArray2 + %68 = OpImageFetch %v4float %67 %66 Lod %int_0 + OpBranch %39 + %44 = OpLabel + %69 = OpCompositeExtract %float %34 0 + %70 = OpCompositeExtract %float %34 1 + %71 = OpConvertFToS %int %69 + %72 = OpConvertFToS %int %70 + %73 = OpCompositeConstruct %v3int %71 %72 %int_0 + %74 = OpVectorShuffle %v2int %73 %73 0 1 + %75 = OpLoad %type_2d_image %g_textureArray3 + %76 = OpImageFetch %v4float %75 %74 Lod %int_0 + OpBranch %39 + %40 = OpLabel + OpBranch %39 + %39 = OpLabel + %77 = OpPhi %v4float %52 %41 %60 %42 %68 %43 %76 %44 %32 %40 + %78 = OpPhi %bool %true %41 %true %42 %true %43 %true %44 %false %40 + OpSelectionMerge %79 None + OpBranchConditional %78 %35 %79 + %79 = OpLabel + OpBranch %35 + %35 = OpLabel + %80 = OpPhi %v4float %77 %39 %18 %79 + OpStore %out_var_SV_TARGET %80 + OpReturn + OpFunctionEnd diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index 23f4f30f0..c085f2d84 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -12740,6 +12740,29 @@ void CompilerGLSL::branch(BlockID from, uint32_t cond, BlockID true_block, Block bool true_block_is_selection_merge = true_block == merge_block; bool false_block_is_selection_merge = false_block == merge_block; + // Can happen if one branch merges to selection branch merge target, + // and then the other branches to outer switch merge target. + if (!true_sub && !false_sub) + { + if (true_block_is_selection_merge && false_block_is_selection_merge) + { + // Useless case (should just be OpBranch), just flush PHI once if needed and execution will continue + // at the merge target. + if (flush_phi_required(from, merge_block)) + flush_phi(from, merge_block); + return; + } + else if (true_block_is_selection_merge) + false_sub = true; + else if (false_block_is_selection_merge) + true_sub = true; + else + { + false_sub = true; + true_sub = true; + } + } + if (true_sub) { emit_block_hints(get(from)); From 2144274a91ae38bc67b1275f44a967f870de1328 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Thu, 17 Sep 2020 12:12:37 +0200 Subject: [PATCH 2/2] Clean up conditional branch codegen. Should only need to look at whether or not we're branching to our own merge target. Any other branch needs to emit code in some way. --- spirv_glsl.cpp | 68 +++++++------------------------------------------- 1 file changed, 9 insertions(+), 59 deletions(-) diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index c085f2d84..b17c0e9f6 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -12732,87 +12732,37 @@ void CompilerGLSL::branch(BlockID from, uint32_t cond, BlockID true_block, Block auto &from_block = get(from); BlockID merge_block = from_block.merge == SPIRBlock::MergeSelection ? from_block.next_block : BlockID(0); - // If we branch directly to a selection merge target, we don't need a code path. - // This covers both merge out of if () / else () as well as a break for switch blocks. - bool true_sub = !is_conditional(true_block); - bool false_sub = !is_conditional(false_block); + // If we branch directly to our selection merge target, we don't need a code path. + bool true_block_needs_code = true_block != merge_block || flush_phi_required(from, true_block); + bool false_block_needs_code = false_block != merge_block || flush_phi_required(from, false_block); - bool true_block_is_selection_merge = true_block == merge_block; - bool false_block_is_selection_merge = false_block == merge_block; + if (!true_block_needs_code && !false_block_needs_code) + return; - // Can happen if one branch merges to selection branch merge target, - // and then the other branches to outer switch merge target. - if (!true_sub && !false_sub) - { - if (true_block_is_selection_merge && false_block_is_selection_merge) - { - // Useless case (should just be OpBranch), just flush PHI once if needed and execution will continue - // at the merge target. - if (flush_phi_required(from, merge_block)) - flush_phi(from, merge_block); - return; - } - else if (true_block_is_selection_merge) - false_sub = true; - else if (false_block_is_selection_merge) - true_sub = true; - else - { - false_sub = true; - true_sub = true; - } - } + emit_block_hints(get(from)); - if (true_sub) + if (true_block_needs_code) { - emit_block_hints(get(from)); statement("if (", to_expression(cond), ")"); begin_scope(); branch(from, true_block); end_scope(); - // If we merge to continue, we handle that explicitly in emit_block_chain(), - // so there is no need to branch to it directly here. - // break; is required to handle ladder fallthrough cases, so keep that in for now, even - // if we could potentially handle it in emit_block_chain(). - if (false_sub || (!false_block_is_selection_merge && is_continue(false_block)) || is_break(false_block)) + if (false_block_needs_code) { statement("else"); begin_scope(); branch(from, false_block); end_scope(); } - else if (flush_phi_required(from, false_block)) - { - statement("else"); - begin_scope(); - flush_phi(from, false_block); - end_scope(); - } } - else if (false_sub) + else if (false_block_needs_code) { // Only need false path, use negative conditional. - emit_block_hints(get(from)); statement("if (!", to_enclosed_expression(cond), ")"); begin_scope(); branch(from, false_block); end_scope(); - - if ((!true_block_is_selection_merge && is_continue(true_block)) || is_break(true_block)) - { - statement("else"); - begin_scope(); - branch(from, true_block); - end_scope(); - } - else if (flush_phi_required(from, true_block)) - { - statement("else"); - begin_scope(); - flush_phi(from, true_block); - end_scope(); - } } }