Fix various corner cases with expression dependencies.

There was a potential problem if variables were invalidated and SPIR-V read expressions which depended on other expression which in turn depended on the invalidated variable. Also fixes issue where variables were considered immutable if they were forwardable. This allowed some incorrect optimizations to slip through.
2016-07-12 14:33:04 +02:00 · 2016-07-12 14:33:04 +02:00 · 36a0b63f28
--- a/reference/shaders/asm/comp/bitcast_iadd.asm.comp
+++ b/reference/shaders/asm/comp/bitcast_iadd.asm.comp
@ -1,13 +1,13 @@
 #version 310 es
 layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;

-layout(binding = 0, std430) buffer _3
+layout(binding = 0, std430) restrict buffer _3
 {
    ivec4 _0;
    uvec4 _1;
 } _5;

-layout(binding = 1, std430) buffer _4
+layout(binding = 1, std430) restrict buffer _4
 {
    uvec4 _0;
    ivec4 _1;
--- a/reference/shaders/asm/frag/invalidation.asm.frag
+++ b/reference/shaders/asm/frag/invalidation.asm.frag
@ -0,0 +1,15 @@
+#version 450
+
+in float v0;
+in float v1;
+out float FragColor;
+
+void main()
+{
+    float a = v0;
+    float b = v1;
+    float _17 = a;
+    a = v1;
+    FragColor = ((_17 + b) * b);
+}
+
--- a/reference/shaders/comp/generate_height.comp
+++ b/reference/shaders/comp/generate_height.comp
@ -28,6 +28,7 @@ uvec2 workaround_mix(uvec2 a, uvec2 b, bvec2 sel)
    {
        _137 = a.x;
    }
+    uint _147 = _137;
    if (sel.y)
    {
        _148 = b.y;
@ -36,7 +37,7 @@ uvec2 workaround_mix(uvec2 a, uvec2 b, bvec2 sel)
    {
        _148 = a.y;
    }
-    return uvec2(_137, _148);
+    return uvec2(_147, _148);
 }

 vec2 alias(vec2 i, vec2 N)
--- a/reference/shaders/comp/torture-loop.comp
+++ b/reference/shaders/comp/torture-loop.comp
@ -21,7 +21,8 @@ void main()
    uint j;
    for (;;)
    {
-        int _40 = k + 1;
+        int _39 = k;
+        int _40 = _39 + 1;
        k = _40;
        if ((_40 < 10))
        {
--- a/reference/shaders/tesc/water_tess.tesc
+++ b/reference/shaders/tesc/water_tess.tesc
@ -26,7 +26,8 @@ bool frustum_cull(vec2 p0)
    float radius = (0.5 * length((bb_max - bb_min)));
    vec3 f0 = vec3(dot(_41.uFrustum[0], vec4(center, 1.0)), dot(_41.uFrustum[1], vec4(center, 1.0)), dot(_41.uFrustum[2], vec4(center, 1.0)));
    vec3 f1 = vec3(dot(_41.uFrustum[3], vec4(center, 1.0)), dot(_41.uFrustum[4], vec4(center, 1.0)), dot(_41.uFrustum[5], vec4(center, 1.0)));
-    bool _205 = any(lessThanEqual(f0, vec3((-radius))));
+    vec3 _199 = f0;
+    bool _205 = any(lessThanEqual(_199, vec3((-radius))));
    bool _215;
    if ((!_205))
    {
--- a/reference/shaders/vert/ground.vert
+++ b/reference/shaders/vert/ground.vert
@ -69,6 +69,7 @@ vec2 warp_position()
    {
        _332 = 0u;
    }
+    uint _342 = _332;
    if ((uPosition.y < 32u))
    {
        _343 = mask.y;
@ -77,7 +78,7 @@ vec2 warp_position()
    {
        _343 = 0u;
    }
-    rounding = uvec2(_332, _343);
+    rounding = uvec2(_342, _343);
    lower_upper_snapped = vec4(((uPosition + rounding).xyxy & (~mask).xxyy));
    return mix(lower_upper_snapped.xy, lower_upper_snapped.zw, vec2(fract_lod));
 }
--- a/shaders/asm/comp/bitcast_iadd.asm.comp
+++ b/shaders/asm/comp/bitcast_iadd.asm.comp
@ -18,9 +18,11 @@
               OpDecorate %input_struct BufferBlock
               OpDecorate %inputs DescriptorSet 0
               OpDecorate %inputs Binding 0
+			   OpDecorate %inputs Restrict
               OpDecorate %output_struct BufferBlock
               OpDecorate %outputs DescriptorSet 0
               OpDecorate %outputs Binding 1
+			   OpDecorate %outputs Restrict

          %void = OpTypeVoid
          %main_func = OpTypeFunction %void
--- a/shaders/asm/frag/invalidation.asm.frag
+++ b/shaders/asm/frag/invalidation.asm.frag
@ -0,0 +1,43 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 1
+; Bound: 28
+; Schema: 0
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main" %v0 %v1 %FragColor
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource GLSL 450
+               OpName %4 "main"
+               OpName %a "a"
+               OpName %v0 "v0"
+               OpName %b "b"
+               OpName %v1 "v1"
+               OpName %FragColor "FragColor"
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+          %float = OpTypeFloat 32
+          %pfloat = OpTypePointer Function %float
+          %9 = OpTypePointer Input %float
+         %v0 = OpVariable %9 Input
+         %v1 = OpVariable %9 Input
+         %25 = OpTypePointer Output %float
+         %FragColor = OpVariable %25 Output
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+         %a = OpVariable %pfloat Function
+         %b = OpVariable %pfloat Function
+         %v0_tmp = OpLoad %float %v0
+         %v1_tmp = OpLoad %float %v1
+               OpStore %a %v0_tmp
+               OpStore %b %v1_tmp
+
+	     %a_tmp = OpLoad %float %a
+	     %b_tmp = OpLoad %float %b
+         %res = OpFAdd %float %a_tmp %b_tmp
+		 %res1 = OpFMul %float %res %b_tmp
+               OpStore %a %v1_tmp
+               OpStore %FragColor %res1
+               OpReturn
+               OpFunctionEnd
--- a/spirv_common.hpp
+++ b/spirv_common.hpp
@ -258,13 +258,15 @@ struct SPIRExpression : IVariant

 	// If this expression will never change, we can avoid lots of temporaries
 	// in high level source.
+	// An expression being immutable can be speculative,
+	// it is assumed that this is true almost always.
 	bool immutable = false;

 	// If this expression has been used while invalidated.
 	bool used_while_invalidated = false;

-	// A list of a variables for which this expression was invalidated by.
-	std::vector<uint32_t> invalidated_by;
+	// A list of expressions which this expression depends on.
+	std::vector<uint32_t> expression_dependencies;
 };

 struct SPIRFunctionPrototype : IVariant
--- a/spirv_cross.cpp
+++ b/spirv_cross.cpp
@ -60,7 +60,8 @@ bool Compiler::variable_storage_is_aliased(const SPIRVariable &v)
 	bool ssbo = (meta[type.self].decoration.decoration_flags & (1ull << DecorationBufferBlock)) != 0;
 	bool image = type.basetype == SPIRType::Image;
 	bool counter = type.basetype == SPIRType::AtomicCounter;
-	return ssbo || image || counter;
+	bool restrict = (meta[v.self].decoration.decoration_flags & (1ull << DecorationRestrict)) != 0;
+	return !restrict && (ssbo || image || counter);
 }

 bool Compiler::block_is_pure(const SPIRBlock &block)
@ -274,10 +275,7 @@ void Compiler::register_write(uint32_t chain)
 void Compiler::flush_dependees(SPIRVariable &var)
 {
 	for (auto expr : var.dependees)
-	{
 		invalid_expressions.insert(expr);
-		get<SPIRExpression>(expr).invalidated_by.push_back(var.self);
-	}
 	var.dependees.clear();
 }

@ -352,7 +350,7 @@ bool Compiler::is_immutable(uint32_t id) const

 		// Anything we load from the UniformConstant address space is guaranteed to be immutable.
 		bool pointer_to_const = var.storage == StorageClassUniformConstant;
-		return pointer_to_const || var.phi_variable || var.forwardable || !expression_is_lvalue(id);
+		return pointer_to_const || var.phi_variable || !expression_is_lvalue(id);
 	}
 	else if (ids[id].get_type() == TypeExpression)
 		return get<SPIRExpression>(id).immutable;
@ -2024,3 +2022,21 @@ uint32_t Compiler::get_subpass_input_remapped_components(uint32_t id) const
 {
 	return get<SPIRVariable>(id).remapped_components;
 }
+
+void Compiler::inherit_expression_dependencies(uint32_t dst, uint32_t source_expression)
+{
+	auto &e = get<SPIRExpression>(dst);
+	auto *s = maybe_get<SPIRExpression>(source_expression);
+	if (!s)
+		return;
+
+	auto &e_deps = e.expression_dependencies;
+	auto &s_deps = s->expression_dependencies;
+
+	// If we depend on a expression, we also depend on all sub-dependencies from source.
+	e_deps.push_back(source_expression);
+	e_deps.insert(end(e_deps), begin(s_deps), end(s_deps));
+
+	// Eliminate duplicated dependencies.
+	e_deps.erase(unique(begin(e_deps), end(e_deps)), end(e_deps));
+}
--- a/spirv_cross.hpp
+++ b/spirv_cross.hpp
@ -349,6 +349,7 @@ protected:
 	uint32_t increase_bound_by(uint32_t incr_amount);

 	bool types_are_logically_equivalent(const SPIRType &a, const SPIRType &b) const;
+	void inherit_expression_dependencies(uint32_t dst, uint32_t source);

 private:
 	void parse();
--- a/spirv_glsl.cpp
+++ b/spirv_glsl.cpp
@ -872,7 +872,8 @@ void CompilerGLSL::emit_push_constant_block_glsl(const SPIRVariable &var)
 void CompilerGLSL::emit_buffer_block(const SPIRVariable &var)
 {
 	auto &type = get<SPIRType>(var.basetype);
-	auto ssbo = meta[type.self].decoration.decoration_flags & (1ull << DecorationBufferBlock);
+	bool ssbo = (meta[type.self].decoration.decoration_flags & (1ull << DecorationBufferBlock)) != 0;
+	bool restrict = (meta[var.self].decoration.decoration_flags & (1ull << DecorationRestrict)) != 0;

 	add_resource_name(var.self);

@ -886,7 +887,7 @@ void CompilerGLSL::emit_buffer_block(const SPIRVariable &var)
 	else
 		resource_names.insert(buffer_name);

-	statement(layout_for_variable(var) + (ssbo ? "buffer " : "uniform ") + buffer_name);
+	statement(layout_for_variable(var), restrict ? "restrict " : "", ssbo ? "buffer " : "uniform ", buffer_name);
 	begin_scope();

 	type.member_name_cache.clear();
@ -1176,33 +1177,44 @@ void CompilerGLSL::emit_resources()
 		statement("");
 }

+void CompilerGLSL::handle_invalid_expression(uint32_t id)
+{
+	auto &expr = get<SPIRExpression>(id);
+
+	// This expression has been invalidated in the past.
+	// Be careful with this expression next pass ...
+	// Used for OpCompositeInsert forwarding atm.
+	expr.used_while_invalidated = true;
+
+	// We tried to read an invalidated expression.
+	// This means we need another pass at compilation, but next time, force temporary variables so that they cannot be invalidated.
+	forced_temporaries.insert(id);
+	force_recompile = true;
+}
+
 string CompilerGLSL::to_expression(uint32_t id)
 {
 	auto itr = invalid_expressions.find(id);
 	if (itr != end(invalid_expressions))
+		handle_invalid_expression(id);
+
+	if (ids[id].get_type() == TypeExpression)
 	{
+		// We might have a more complex chain of dependencies.
+		// A possible scenario is that we
+		//
+		// %1 = OpLoad
+		// %2 = OpDoSomething %1 %1. here %2 will have a dependency on %1.
+		// %3 = OpDoSomethingAgain %2 %2. Here %3 will lose the link to %1 since we don't propagate the dependencies like that.
+		// OpStore %1 %foo // Here we can invalidate %1, and hence all expressions which depend on %1. Only %2 will know since it's part of invalid_expressions.
+		// %4 = OpDoSomethingAnotherTime %3 %3 // If we forward all expressions we will see %1 expression after store, not before.
+		//
+		// However, we can propagate up a list of depended expressions when we used %2, so we can check if %2 is invalid when reading %3 after the store,
+		// and see that we should not forward reads of the original variable.
 		auto &expr = get<SPIRExpression>(id);
-
-		// This expression has been invalidated in the past.
-		// Be careful with this expression next pass ...
-		// Used for OpCompositeInsert forwarding atm.
-		expr.used_while_invalidated = true;
-
-		// We tried to read an invalidated expression.
-		// This means we need another pass at compilation, but next time, do not try to forward
-		// the variables which caused invalidation to happen in the first place.
-		for (auto var : expr.invalidated_by)
-		{
-			//fprintf(stderr, "Expression %u was invalidated due to variable %u being invalid at read time!\n", id, var);
-			get<SPIRVariable>(var).forwardable = false;
-		}
-
-		if (expr.invalidated_by.empty() && expr.loaded_from)
-		{
-			//fprintf(stderr, "Expression %u was invalidated due to variable %u being invalid at read time!\n", id, expr.loaded_from);
-			get<SPIRVariable>(expr.loaded_from).forwardable = false;
-		}
-		force_recompile = true;
+		for (uint32_t dep : expr.expression_dependencies)
+			if (invalid_expressions.find(dep) != end(invalid_expressions))
+				handle_invalid_expression(dep);
 	}

 	track_expression_read(id);
@ -1441,13 +1453,23 @@ SPIRExpression &CompilerGLSL::emit_op(uint32_t result_type, uint32_t result_id,

 void CompilerGLSL::emit_unary_op(uint32_t result_type, uint32_t result_id, uint32_t op0, const char *op)
 {
-	emit_op(result_type, result_id, join(op, to_expression(op0)), should_forward(op0), true);
+	bool forward = should_forward(op0);
+	emit_op(result_type, result_id, join(op, to_expression(op0)), forward, true);
+
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+		inherit_expression_dependencies(result_id, op0);
 }

 void CompilerGLSL::emit_binary_op(uint32_t result_type, uint32_t result_id, uint32_t op0, uint32_t op1, const char *op)
 {
-	emit_op(result_type, result_id, join(to_expression(op0), " ", op, " ", to_expression(op1)),
-	        should_forward(op0) && should_forward(op1), true);
+	bool forward = should_forward(op0) && should_forward(op1);
+	emit_op(result_type, result_id, join(to_expression(op0), " ", op, " ", to_expression(op1)), forward, true);
+
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+	{
+		inherit_expression_dependencies(result_id, op0);
+		inherit_expression_dependencies(result_id, op1);
+	}
 }

 SPIRType CompilerGLSL::binary_op_bitcast_helper(string &cast_op0, string &cast_op1, SPIRType::BaseType &input_type,
@ -1516,14 +1538,23 @@ void CompilerGLSL::emit_binary_op_cast(uint32_t result_type, uint32_t result_id,

 void CompilerGLSL::emit_unary_func_op(uint32_t result_type, uint32_t result_id, uint32_t op0, const char *op)
 {
-	emit_op(result_type, result_id, join(op, "(", to_expression(op0), ")"), should_forward(op0), false);
+	bool forward = should_forward(op0);
+	emit_op(result_type, result_id, join(op, "(", to_expression(op0), ")"), forward, false);
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+		inherit_expression_dependencies(result_id, op0);
 }

 void CompilerGLSL::emit_binary_func_op(uint32_t result_type, uint32_t result_id, uint32_t op0, uint32_t op1,
                                       const char *op)
 {
-	emit_op(result_type, result_id, join(op, "(", to_expression(op0), ", ", to_expression(op1), ")"),
-	        should_forward(op0) && should_forward(op1), false);
+	bool forward = should_forward(op0) && should_forward(op1);
+	emit_op(result_type, result_id, join(op, "(", to_expression(op0), ", ", to_expression(op1), ")"), forward, false);
+
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+	{
+		inherit_expression_dependencies(result_id, op0);
+		inherit_expression_dependencies(result_id, op1);
+	}
 }

 void CompilerGLSL::emit_binary_func_op_cast(uint32_t result_type, uint32_t result_id, uint32_t op0, uint32_t op1,
@ -1554,17 +1585,33 @@ void CompilerGLSL::emit_binary_func_op_cast(uint32_t result_type, uint32_t resul
 void CompilerGLSL::emit_trinary_func_op(uint32_t result_type, uint32_t result_id, uint32_t op0, uint32_t op1,
                                        uint32_t op2, const char *op)
 {
+	bool forward = should_forward(op0) && should_forward(op1) && should_forward(op2);
 	emit_op(result_type, result_id,
-	        join(op, "(", to_expression(op0), ", ", to_expression(op1), ", ", to_expression(op2), ")"),
-	        should_forward(op0) && should_forward(op1) && should_forward(op2), false);
+	        join(op, "(", to_expression(op0), ", ", to_expression(op1), ", ", to_expression(op2), ")"), forward, false);
+
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+	{
+		inherit_expression_dependencies(result_id, op0);
+		inherit_expression_dependencies(result_id, op1);
+		inherit_expression_dependencies(result_id, op2);
+	}
 }

 void CompilerGLSL::emit_quaternary_func_op(uint32_t result_type, uint32_t result_id, uint32_t op0, uint32_t op1,
                                           uint32_t op2, uint32_t op3, const char *op)
 {
+	bool forward = should_forward(op0) && should_forward(op1) && should_forward(op2) && should_forward(op3);
 	emit_op(result_type, result_id, join(op, "(", to_expression(op0), ", ", to_expression(op1), ", ",
 	                                     to_expression(op2), ", ", to_expression(op3), ")"),
-	        should_forward(op0) && should_forward(op1) && should_forward(op2) && should_forward(op3), false);
+	        forward, false);
+
+	if (forward && forced_temporaries.find(result_id) == end(forced_temporaries))
+	{
+		inherit_expression_dependencies(result_id, op0);
+		inherit_expression_dependencies(result_id, op1);
+		inherit_expression_dependencies(result_id, op2);
+		inherit_expression_dependencies(result_id, op3);
+	}
 }

 string CompilerGLSL::legacy_tex_op(const std::string &op, const SPIRType &imgtype)
@ -2391,7 +2438,11 @@ string CompilerGLSL::access_chain(uint32_t base, const uint32_t *indices, uint32

 bool CompilerGLSL::should_forward(uint32_t id)
 {
-	return is_immutable(id) && !options.force_temporary;
+	// Immutable expression can always be forwarded.
+	// If not immutable, we can speculate about it by forwarding potentially mutable variables.
+	auto *var = maybe_get<SPIRVariable>(id);
+	bool forward = var ? var->forwardable : false;
+	return (is_immutable(id) || forward) && !options.force_temporary;
 }

 void CompilerGLSL::track_expression_read(uint32_t id)
@ -2665,18 +2716,12 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)

 		// If we're loading from memory that cannot be changed by the shader,
 		// just forward the expression directly to avoid needless temporaries.
-		if (should_forward(ptr))
-		{
-			set<SPIRExpression>(id, to_expression(ptr), result_type, true);
-			register_read(id, ptr, true);
-		}
-		else
-		{
-			// If the variable can be modified after this OpLoad, we cannot just forward the expression.
-			// We must read it now and store it in a temporary.
-			emit_op(result_type, id, to_expression(ptr), false, false);
-			register_read(id, ptr, false);
-		}
+		// If an expression is mutable and forwardable, we speculate that it is immutable.
+		bool forward = should_forward(ptr) && forced_temporaries.find(id) == end(forced_temporaries);
+
+		// Suppress usage tracking since using same expression multiple times does not imply any extra work.
+		emit_op(result_type, id, to_expression(ptr), forward, false, true);
+		register_read(id, ptr, forward);
 		break;
 	}

@ -2688,8 +2733,9 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 			flush_variable_declaration(var->self);

 		// If the base is immutable, the access chain pointer must also be.
+		// If an expression is mutable and forwardable, we speculate that it is immutable.
 		auto e = access_chain(ops[2], &ops[3], length - 3, false);
-		auto &expr = set<SPIRExpression>(ops[1], move(e), ops[0], is_immutable(ops[2]));
+		auto &expr = set<SPIRExpression>(ops[1], move(e), ops[0], should_forward(ops[2]));
 		expr.loaded_from = ops[2];
 		break;
 	}
@ -2709,8 +2755,8 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 			// For this case, we don't need to invalidate anything and emit any opcode.
 			if (lhs != rhs)
 			{
-				register_write(ops[0]);
 				statement(lhs, " = ", rhs, ";");
+				register_write(ops[0]);
 			}
 		}
 		break;
--- a/spirv_glsl.hpp
+++ b/spirv_glsl.hpp
@ -345,6 +345,8 @@ protected:
 	void add_variable(std::unordered_set<std::string> &variables, uint32_t id);

 	void check_function_call_constraints(const uint32_t *args, uint32_t length);
+
+	void handle_invalid_expression(uint32_t id);
 };
 }