Merge pull request #1099 from KhronosGroup/fix-1091

HansKristian-Work · web-flow · commit 301eab1b7a75 · 2019-07-25T17:44:17.000+02:00
Missed case where DoWhile continue block deals with Phi.
diff --git a/reference/opt/shaders/asm/comp/phi-temporary-copy-loop-variable.asm.comp b/reference/opt/shaders/asm/comp/phi-temporary-copy-loop-variable.asm.comp
@@ -14,10 +14,6 @@ void main()
         {
             break;
         }
-        else
-        {
-            continue;
-        }
         continue;
     }
     imageStore(outImageTexture, ivec2(gl_GlobalInvocationID.xy), vec4(float(_30 - 1), float(_30), 1.0, 1.0));
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
@@ -46,14 +46,7 @@ void main()
                     _231 = true;
                     break;
                 }
-                else
-                {
-                    uint _204 = _227 + uint(1);
-                    _227 = _204;
-                    continue;
-                }
-                uint _204 = _227 + uint(1);
-                _227 = _204;
+                _227++;
                 continue;
             }
             else
diff --git a/reference/shaders-msl-no-opt/asm/frag/inliner-dominator-inside-loop.asm.frag b/reference/shaders-msl-no-opt/asm/frag/inliner-dominator-inside-loop.asm.frag
@@ -137,7 +137,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)
     float2 _166 = in.IN_Uv_EdgeDistance1.xy * 1.0;
     bool _173;
     float4 _193;
-    do
+    for (;;)
     {
         _173 = 0.0 == 0.0;
         if (_173)
@@ -153,9 +153,9 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)
         }
         _193 = _192;
         break;
-    } while (false);
+    }
     float4 _220;
-    do
+    for (;;)
     {
         if (_173)
         {
@@ -170,7 +170,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)
         }
         _220 = _219;
         break;
-    } while (false);
+    }
     float2 _223 = float2(1.0);
     float2 _224 = (_220.wy * 2.0) - _223;
     float3 _232 = float3(_224, sqrt(fast::clamp(1.0 + dot(-_224, _224), 0.0, 1.0)));
@@ -181,7 +181,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)
     float3 _256 = float3(_255.x, _255.y, _253.z);
     float3 _271 = ((in.IN_Color.xyz * (_193 * 1.0).xyz) * (1.0 + (_256.x * 0.300000011920928955078125))) * (StudsMapTexture.sample(StudsMapSampler, _156.UvStuds).x * 2.0);
     float4 _298;
-    do
+    for (;;)
     {
         if (0.75 == 0.0)
         {
@@ -196,7 +196,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)
         }
         _298 = _297;
         break;
-    } while (false);
+    }
     float2 _303 = mix(float2(0.800000011920928955078125, 120.0), (_298.xy * float2(2.0, 256.0)) + float2(0.0, 0.00999999977648258209228515625), float2(_165));
     Surface _304 = _125;
     _304.albedo = _271;
diff --git a/reference/shaders-no-opt/asm/frag/do-while-continue-phi.asm.frag b/reference/shaders-no-opt/asm/frag/do-while-continue-phi.asm.frag
@@ -0,0 +1,37 @@
+#version 310 es
+precision mediump float;
+precision highp int;
+
+layout(location = 0) out highp vec4 _GLF_color;
+
+void main()
+{
+    for (;;)
+    {
+        bool _32;
+        for (;;)
+        {
+            if (gl_FragCoord.x != gl_FragCoord.x)
+            {
+                _32 = true;
+                break;
+            }
+            if (false)
+            {
+                continue;
+            }
+            else
+            {
+                _32 = false;
+                break;
+            }
+        }
+        if (_32)
+        {
+            break;
+        }
+        _GLF_color = vec4(1.0, 0.0, 0.0, 1.0);
+        break;
+    }
+}
+
diff --git a/reference/shaders-no-opt/asm/frag/inliner-dominator-inside-loop.asm.frag b/reference/shaders-no-opt/asm/frag/inliner-dominator-inside-loop.asm.frag
@@ -130,7 +130,7 @@ void main()
     vec2 _166 = IN_Uv_EdgeDistance1.xy * 1.0;
     bool _173;
     vec4 _193;
-    do
+    for (;;)
     {
         _173 = 0.0 == 0.0;
         if (_173)
@@ -146,9 +146,9 @@ void main()
         }
         _193 = _192;
         break;
-    } while (false);
+    }
     vec4 _220;
-    do
+    for (;;)
     {
         if (_173)
         {
@@ -163,7 +163,7 @@ void main()
         }
         _220 = _219;
         break;
-    } while (false);
+    }
     vec2 _223 = vec2(1.0);
     vec2 _224 = (_220.wy * 2.0) - _223;
     vec3 _232 = vec3(_224, sqrt(clamp(1.0 + dot(-_224, _224), 0.0, 1.0)));
@@ -174,7 +174,7 @@ void main()
     vec3 _256 = vec3(_255.x, _255.y, _253.z);
     vec3 _271 = ((IN_Color.xyz * (_193 * 1.0).xyz) * (1.0 + (_256.x * 0.300000011920928955078125))) * (texture(SPIRV_Cross_CombinedStudsMapTextureStudsMapSampler, _156.UvStuds).x * 2.0);
     vec4 _298;
-    do
+    for (;;)
     {
         if (0.75 == 0.0)
         {
@@ -189,7 +189,7 @@ void main()
         }
         _298 = _297;
         break;
-    } while (false);
+    }
     vec2 _303 = mix(vec2(0.800000011920928955078125, 120.0), (_298.xy * vec2(2.0, 256.0)) + vec2(0.0, 0.00999999977648258209228515625), vec2(_165));
     Surface _304 = _125;
     _304.albedo = _271;
diff --git a/reference/shaders/asm/comp/phi-temporary-copy-loop-variable.asm.comp b/reference/shaders/asm/comp/phi-temporary-copy-loop-variable.asm.comp
@@ -14,10 +14,6 @@ void main()
         {
             break;
         }
-        else
-        {
-            continue;
-        }
         continue;
     }
     imageStore(outImageTexture, ivec2(gl_GlobalInvocationID.xy), vec4(float(_30 - 1), float(_30), 1.0, 1.0));
diff --git a/shaders-no-opt/asm/frag/do-while-continue-phi.asm.frag b/shaders-no-opt/asm/frag/do-while-continue-phi.asm.frag
@@ -0,0 +1,64 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 7
+; Bound: 42
+; 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
+      %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
+      %false = OpConstantFalse %bool
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+ %_GLF_color = OpVariable %_ptr_Output_v4float Output
+    %float_1 = OpConstant %float 1
+    %float_0 = OpConstant %float 0
+         %31 = OpConstantComposite %v4float %float_1 %float_0 %float_0 %float_1
+       %true = OpConstantTrue %bool
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+               OpBranch %33
+         %33 = OpLabel
+               OpLoopMerge %32 %35 None
+               OpBranch %6
+          %6 = OpLabel
+               OpLoopMerge %8 %24 None
+               OpBranch %7
+          %7 = 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 %8
+         %24 = OpLabel
+               OpBranchConditional %false %6 %8
+          %8 = OpLabel
+         %41 = OpPhi %bool %true %23 %false %24
+               OpSelectionMerge %39 None
+               OpBranchConditional %41 %32 %39
+         %39 = OpLabel
+               OpStore %_GLF_color %31
+               OpBranch %32
+         %35 = OpLabel
+               OpBranch %33
+         %32 = OpLabel
+               OpReturn
+               OpFunctionEnd
diff --git a/spirv_cross.cpp b/spirv_cross.cpp
@@ -1501,6 +1501,12 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc
 		const auto *true_block = maybe_get<SPIRBlock>(block.true_block);
 		const auto *merge_block = maybe_get<SPIRBlock>(dominator.merge_block);
 
+		// If we need to flush Phi in this block, we cannot have a DoWhile loop.
+		bool flush_phi_to_false = false_block && flush_phi_required(block.self, block.false_block);
+		bool flush_phi_to_true = true_block && flush_phi_required(block.self, block.true_block);
+		if (flush_phi_to_false || flush_phi_to_true)
+			return SPIRBlock::ComplexLoop;
+
 		bool positive_do_while = block.true_block == dominator.self &&
 		                         (block.false_block == dominator.merge_block ||
 		                          (false_block && merge_block && execution_is_noop(*false_block, *merge_block)));
@@ -4246,3 +4252,12 @@ bool Compiler::type_is_array_of_pointers(const SPIRType &type) const
 	// If parent type has same pointer depth, we must have an array of pointers.
 	return type.pointer_depth == get<SPIRType>(type.parent_type).pointer_depth;
 }
+
+bool Compiler::flush_phi_required(uint32_t from, uint32_t to) const
+{
+	auto &child = get<SPIRBlock>(to);
+	for (auto &phi : child.phi_variables)
+		if (phi.parent == from)
+			return true;
+	return false;
+}
diff --git a/spirv_cross.hpp b/spirv_cross.hpp
@@ -972,6 +972,8 @@ class Compiler
 	bool reflection_ssbo_instance_name_is_significant() const;
 	std::string get_remapped_declared_block_name(uint32_t id, bool fallback_prefer_instance_name) const;
 
+	bool flush_phi_required(uint32_t from, uint32_t to) const;
+
 private:
 	// Used only to implement the old deprecated get_entry_point() interface.
 	const SPIREntryPoint &get_first_entry_point(const std::string &name) const;
diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp
@@ -11068,15 +11068,6 @@ void CompilerGLSL::emit_fixup()
 	}
 }
 
-bool CompilerGLSL::flush_phi_required(uint32_t from, uint32_t to)
-{
-	auto &child = get<SPIRBlock>(to);
-	for (auto &phi : child.phi_variables)
-		if (phi.parent == from)
-			return true;
-	return false;
-}
-
 void CompilerGLSL::flush_phi(uint32_t from, uint32_t to)
 {
 	auto &child = get<SPIRBlock>(to);
@@ -11246,10 +11237,17 @@ void CompilerGLSL::branch(uint32_t from, uint32_t to)
 
 void CompilerGLSL::branch(uint32_t from, uint32_t cond, uint32_t true_block, uint32_t false_block)
 {
-	// If we branch directly to a selection merge target, we don't really need a code path.
+	auto &from_block = get<SPIRBlock>(from);
+	uint32_t merge_block = from_block.merge == SPIRBlock::MergeSelection ? from_block.next_block : 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);
 
+	bool true_block_is_selection_merge = true_block == merge_block;
+	bool false_block_is_selection_merge = false_block == merge_block;
+
 	if (true_sub)
 	{
 		emit_block_hints(get<SPIRBlock>(from));
@@ -11258,7 +11256,11 @@ void CompilerGLSL::branch(uint32_t from, uint32_t cond, uint32_t true_block, uin
 		branch(from, true_block);
 		end_scope();
 
-		if (false_sub || is_continue(false_block) || is_break(false_block))
+		// 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))
 		{
 			statement("else");
 			begin_scope();
@@ -11273,7 +11275,7 @@ void CompilerGLSL::branch(uint32_t from, uint32_t cond, uint32_t true_block, uin
 			end_scope();
 		}
 	}
-	else if (false_sub && !true_sub)
+	else if (false_sub)
 	{
 		// Only need false path, use negative conditional.
 		emit_block_hints(get<SPIRBlock>(from));
@@ -11282,7 +11284,7 @@ void CompilerGLSL::branch(uint32_t from, uint32_t cond, uint32_t true_block, uin
 		branch(from, false_block);
 		end_scope();
 
-		if (is_continue(true_block) || is_break(true_block))
+		if ((!true_block_is_selection_merge && is_continue(true_block)) || is_break(true_block))
 		{
 			statement("else");
 			begin_scope();
diff --git a/spirv_glsl.hpp b/spirv_glsl.hpp
@@ -436,7 +436,6 @@ class CompilerGLSL : public Compiler
 	void branch_to_continue(uint32_t from, uint32_t to);
 	void branch(uint32_t from, uint32_t cond, uint32_t true_block, uint32_t false_block);
 	void flush_phi(uint32_t from, uint32_t to);
-	bool flush_phi_required(uint32_t from, uint32_t to);
 	void flush_variable_declaration(uint32_t id);
 	void flush_undeclared_variables(SPIRBlock &block);
 	void emit_variable_temporary_copies(const SPIRVariable &var);

Original file line number	Diff line number	Diff line change
`@@ -14,10 +14,6 @@ void main()`
`14`	`14`	`{`
`15`	`15`	`break;`
`16`	`16`	`}`
`17`		`- else`
`18`		`- {`
`19`		`- continue;`
`20`		`- }`
`21`	`17`	`continue;`
`22`	`18`	`}`
`23`	`19`	`imageStore(outImageTexture, ivec2(gl_GlobalInvocationID.xy), vec4(float(_30 - 1), float(_30), 1.0, 1.0));`
Original file line number	Diff line number	Diff line change
`@@ -137,7 +137,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)`
`137`	`137`	`float2 _166 = in.IN_Uv_EdgeDistance1.xy * 1.0;`
`138`	`138`	`bool _173;`
`139`	`139`	`float4 _193;`
`140`		`- do`
	`140`	`+ for (;;)`
`141`	`141`	`{`
`142`	`142`	`_173 = 0.0 == 0.0;`
`143`	`143`	`if (_173)`
`@@ -153,9 +153,9 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)`
`153`	`153`	`}`
`154`	`154`	`_193 = _192;`
`155`	`155`	`break;`
`156`		`- } while (false);`
	`156`	`+ }`
`157`	`157`	`float4 _220;`
`158`		`- do`
	`158`	`+ for (;;)`
`159`	`159`	`{`
`160`	`160`	`if (_173)`
`161`	`161`	`{`
`@@ -170,7 +170,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)`
`170`	`170`	`}`
`171`	`171`	`_220 = _219;`
`172`	`172`	`break;`
`173`		`- } while (false);`
	`173`	`+ }`
`174`	`174`	`float2 _223 = float2(1.0);`
`175`	`175`	`float2 _224 = (_220.wy * 2.0) - _223;`
`176`	`176`	`float3 _232 = float3(_224, sqrt(fast::clamp(1.0 + dot(-_224, _224), 0.0, 1.0)));`
`@@ -181,7 +181,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)`
`181`	`181`	`float3 _256 = float3(_255.x, _255.y, _253.z);`
`182`	`182`	`float3 _271 = ((in.IN_Color.xyz * (_193 * 1.0).xyz) * (1.0 + (_256.x * 0.300000011920928955078125))) * (StudsMapTexture.sample(StudsMapSampler, _156.UvStuds).x * 2.0);`
`183`	`183`	`float4 _298;`
`184`		`- do`
	`184`	`+ for (;;)`
`185`	`185`	`{`
`186`	`186`	`if (0.75 == 0.0)`
`187`	`187`	`{`
`@@ -196,7 +196,7 @@ fragment main0_out main0(main0_in in [[stage_in]], constant CB0& _19 [[buffer(0)`
`196`	`196`	`}`
`197`	`197`	`_298 = _297;`
`198`	`198`	`break;`
`199`		`- } while (false);`
	`199`	`+ }`
`200`	`200`	`float2 _303 = mix(float2(0.800000011920928955078125, 120.0), (_298.xy * float2(2.0, 256.0)) + float2(0.0, 0.00999999977648258209228515625), float2(_165));`
`201`	`201`	`Surface _304 = _125;`
`202`	`202`	`_304.albedo = _271;`
Original file line number	Diff line number	Diff line change
`@@ -130,7 +130,7 @@ void main()`
`130`	`130`	`vec2 _166 = IN_Uv_EdgeDistance1.xy * 1.0;`
`131`	`131`	`bool _173;`
`132`	`132`	`vec4 _193;`
`133`		`- do`
	`133`	`+ for (;;)`
`134`	`134`	`{`
`135`	`135`	`_173 = 0.0 == 0.0;`
`136`	`136`	`if (_173)`
`@@ -146,9 +146,9 @@ void main()`
`146`	`146`	`}`
`147`	`147`	`_193 = _192;`
`148`	`148`	`break;`
`149`		`- } while (false);`
	`149`	`+ }`
`150`	`150`	`vec4 _220;`
`151`		`- do`
	`151`	`+ for (;;)`
`152`	`152`	`{`
`153`	`153`	`if (_173)`
`154`	`154`	`{`
`@@ -163,7 +163,7 @@ void main()`
`163`	`163`	`}`
`164`	`164`	`_220 = _219;`
`165`	`165`	`break;`
`166`		`- } while (false);`
	`166`	`+ }`
`167`	`167`	`vec2 _223 = vec2(1.0);`
`168`	`168`	`vec2 _224 = (_220.wy * 2.0) - _223;`
`169`	`169`	`vec3 _232 = vec3(_224, sqrt(clamp(1.0 + dot(-_224, _224), 0.0, 1.0)));`
`@@ -174,7 +174,7 @@ void main()`
`174`	`174`	`vec3 _256 = vec3(_255.x, _255.y, _253.z);`
`175`	`175`	`vec3 _271 = ((IN_Color.xyz * (_193 * 1.0).xyz) * (1.0 + (_256.x * 0.300000011920928955078125))) * (texture(SPIRV_Cross_CombinedStudsMapTextureStudsMapSampler, _156.UvStuds).x * 2.0);`
`176`	`176`	`vec4 _298;`
`177`		`- do`
	`177`	`+ for (;;)`
`178`	`178`	`{`
`179`	`179`	`if (0.75 == 0.0)`
`180`	`180`	`{`
`@@ -189,7 +189,7 @@ void main()`
`189`	`189`	`}`
`190`	`190`	`_298 = _297;`
`191`	`191`	`break;`
`192`		`- } while (false);`
	`192`	`+ }`
`193`	`193`	`vec2 _303 = mix(vec2(0.800000011920928955078125, 120.0), (_298.xy * vec2(2.0, 256.0)) + vec2(0.0, 0.00999999977648258209228515625), vec2(_165));`
`194`	`194`	`Surface _304 = _125;`
`195`	`195`	`_304.albedo = _271;`