Merge pull request #155 from KhronosGroup/cfg-parameter-preserve

Preserve function arguments if variable is only conditionally written.

.travis.yml

@@ -25,5 +25,5 @@ script:
   - make -j2
   - PATH=./glslang/StandAlone:./SPIRV-Tools/tools:$PATH
   - ./test_shaders.py shaders
-  - ./test_shaders.py --metal shaders-msl
+  - ./test_shaders.py --msl shaders-msl
   - ./test_shaders.py --hlsl shaders-hlsl

reference/shaders/comp/cfg-preserve-parameter.comp

@@ -0,0 +1,74 @@
+#version 310 es
+layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
+
+void out_test_0(int cond, out int i)
+{
+    if (cond == 0)
+    {
+        i = 40;
+    }
+    else
+    {
+        i = 60;
+    }
+}
+
+void out_test_1(int cond, out int i)
+{
+    switch (cond)
+    {
+        case 40:
+        {
+            i = 40;
+            break;
+        }
+        default:
+        {
+            i = 70;
+            break;
+        }
+    }
+}
+
+void inout_test_0(int cond, inout int i)
+{
+    if (cond == 0)
+    {
+        i = 40;
+    }
+}
+
+void inout_test_1(int cond, inout int i)
+{
+    switch (cond)
+    {
+        case 40:
+        {
+            i = 40;
+            break;
+        }
+    }
+}
+
+void main()
+{
+    int cond = 40;
+    int i = 50;
+    int param = cond;
+    int param_1 = i;
+    out_test_0(param, param_1);
+    i = param_1;
+    int param_2 = cond;
+    int param_3 = i;
+    out_test_1(param_2, param_3);
+    i = param_3;
+    int param_4 = cond;
+    int param_5 = i;
+    inout_test_0(param_4, param_5);
+    i = param_5;
+    int param_6 = cond;
+    int param_7 = i;
+    inout_test_1(param_6, param_7);
+    i = param_7;
+}
+

shaders/comp/cfg-preserve-parameter.comp

@@ -0,0 +1,54 @@
+#version 310 es
+
+// We write in all paths (and no reads), so should just be out.
+void out_test_0(int cond, inout int i)
+{
+   if (cond == 0)
+      i = 40;
+   else
+      i = 60;
+}
+
+// We write in all paths (and no reads), so should just be out.
+void out_test_1(int cond, inout int i)
+{
+   switch (cond)
+   {
+      case 40:
+         i = 40;
+         break;
+
+      default:
+         i = 70;
+         break;
+   }
+}
+
+// We don't write in all paths, so should be inout.
+void inout_test_0(int cond, inout int i)
+{
+   if (cond == 0)
+      i = 40;
+}
+
+void inout_test_1(int cond, inout int i)
+{
+   switch (cond)
+   {
+      case 40:
+         i = 40;
+         break;
+   }
+}
+
+
+void main()
+{
+   int cond = 40;
+   int i = 50;
+
+   out_test_0(cond, i);
+   out_test_1(cond, i);
+   inout_test_0(cond, i);
+   inout_test_1(cond, i);
+}

spirv_cfg.cpp

@@ -15,6 +15,7 @@
  */
 
 #include "spirv_cfg.hpp"
+#include "spirv_cross.hpp"
 #include <algorithm>
 #include <assert.h>
 

spirv_cfg.hpp

@@ -17,11 +17,12 @@
 #ifndef SPIRV_CROSS_CFG_HPP
 #define SPIRV_CROSS_CFG_HPP
 
-#include "spirv_cross.hpp"
+#include "spirv_common.hpp"
 #include <assert.h>
 
 namespace spirv_cross
 {
+class Compiler;
 class CFG
 {
 public:

spirv_common.hpp

@@ -17,11 +17,21 @@
 #ifndef SPIRV_CROSS_COMMON_HPP
 #define SPIRV_CROSS_COMMON_HPP
 
+#include "spirv.hpp"
+
 #include <cstdio>
 #include <cstring>
 #include <functional>
 #include <locale>
+#include <memory>
 #include <sstream>
+#include <stack>
+#include <stdexcept>
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <utility>
+#include <vector>
 
 namespace spirv_cross
 {

spirv_cross.cpp

@@ -2802,6 +2802,71 @@ const SPIRConstant &Compiler::get_constant(uint32_t id) const
 	return get<SPIRConstant>(id);
 }
 
+static bool exists_unaccessed_path_to_return(const CFG &cfg, uint32_t block, const unordered_set<uint32_t> &blocks)
+{
+	// This block accesses the variable.
+	if (blocks.find(block) != end(blocks))
+		return false;
+
+	// We are at the end of the CFG.
+	if (cfg.get_succeeding_edges(block).empty())
+		return true;
+
+	// If any of our successors have a path to the end, there exists a path from block.
+	for (auto &succ : cfg.get_succeeding_edges(block))
+		if (exists_unaccessed_path_to_return(cfg, succ, blocks))
+			return true;
+
+	return false;
+}
+
+void Compiler::analyze_parameter_preservation(
+    SPIRFunction &entry, const CFG &cfg, const unordered_map<uint32_t, unordered_set<uint32_t>> &variable_to_blocks)
+{
+	for (auto &arg : entry.arguments)
+	{
+		// Non-pointers are always inputs.
+		auto &type = get<SPIRType>(arg.type);
+		if (!type.pointer)
+			continue;
+
+		// Opaque argument types are always in
+		bool potential_preserve;
+		switch (type.basetype)
+		{
+		case SPIRType::Sampler:
+		case SPIRType::Image:
+		case SPIRType::SampledImage:
+		case SPIRType::AtomicCounter:
+			potential_preserve = false;
+			break;
+
+		default:
+			potential_preserve = true;
+			break;
+		}
+
+		if (!potential_preserve)
+			continue;
+
+		auto itr = variable_to_blocks.find(arg.id);
+		if (itr == end(variable_to_blocks))
+		{
+			// Variable is never accessed.
+			continue;
+		}
+
+		// If there is a path through the CFG where no block writes to the variable, the variable will be in an undefined state
+		// when the function returns. We therefore need to implicitly preserve the variable in case there are writers in the function.
+		// Major case here is if a function is
+		// void foo(int &var) { if (cond) var = 10; }
+		// Using read/write counts, we will think it's just an out variable, but it really needs to be inout,
+		// because if we don't write anything whatever we put into the function must return back to the caller.
+		if (exists_unaccessed_path_to_return(cfg, entry.entry_block, itr->second))
+			arg.read_count++;
+	}
+}
+
 void Compiler::analyze_variable_scope(SPIRFunction &entry)
 {
 	struct AccessHandler : OpcodeHandler
@@ -2971,6 +3036,9 @@ void Compiler::analyze_variable_scope(SPIRFunction &entry)
 	// Compute the control flow graph for this function.
 	CFG cfg(*this, entry);
 
+	// Analyze if there are parameters which need to be implicitly preserved with an "in" qualifier.
+	analyze_parameter_preservation(entry, cfg, handler.accessed_variables_to_block);
+
 	unordered_map<uint32_t, uint32_t> potential_loop_variables;
 
 	// For each variable which is statically accessed.

spirv_cross.hpp

@@ -18,19 +18,11 @@
 #define SPIRV_CROSS_HPP
 
 #include "spirv.hpp"
-#include <memory>
-#include <stack>
-#include <stdexcept>
-#include <string>
-#include <unordered_map>
-#include <unordered_set>
-#include <utility>
-#include <vector>
-
 #include "spirv_common.hpp"
 
 namespace spirv_cross
 {
+class CFG;
 struct Resource
 {
 	// Resources are identified with their SPIR-V ID.
@@ -614,6 +606,10 @@ protected:
 	uint64_t active_output_builtins = 0;
 	// Traverses all reachable opcodes and sets active_builtins to a bitmask of all builtin variables which are accessed in the shader.
 	void update_active_builtins();
+
+	void analyze_parameter_preservation(
+	    SPIRFunction &entry, const CFG &cfg,
+	    const std::unordered_map<uint32_t, std::unordered_set<uint32_t>> &variable_to_blocks);
 };
 }
 

spirv_hlsl.cpp

@@ -1404,7 +1404,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "==");
 		else
-			BOP_CAST(== , SPIRType::Int);
+			BOP_CAST(==, SPIRType::Int);
 		break;
 	}
 
@@ -1417,7 +1417,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "==");
 		else
-			BOP(== );
+			BOP(==);
 		break;
 	}
 
@@ -1429,7 +1429,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "!=");
 		else
-			BOP_CAST(!= , SPIRType::Int);
+			BOP_CAST(!=, SPIRType::Int);
 		break;
 	}
 
@@ -1442,7 +1442,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "!=");
 		else
-			BOP(!= );
+			BOP(!=);
 		break;
 	}
 
@@ -1482,7 +1482,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], ">=");
 		else
-			BOP_CAST(>= , type);
+			BOP_CAST(>=, type);
 		break;
 	}
 
@@ -1494,7 +1494,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], ">=");
 		else
-			BOP(>= );
+			BOP(>=);
 		break;
 	}
 
@@ -1534,7 +1534,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "<=");
 		else
-			BOP_CAST(<= , type);
+			BOP_CAST(<=, type);
 		break;
 	}
 
@@ -1546,7 +1546,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction)
 		if (expression_type(ops[2]).vecsize > 1)
 			emit_unrolled_binary_op(result_type, id, ops[2], ops[3], "<=");
 		else
-			BOP(<= );
+			BOP(<=);
 		break;
 	}