Merge pull request #96 from KhronosGroup/transpose-improvement

Legacy GLSL improvements

main.cpp

@@ -27,7 +27,7 @@
 #include <unordered_set>
 
 #ifdef _MSC_VER
-#pragma warning(disable: 4996)
+#pragma warning(disable : 4996)
 #endif
 
 using namespace spv;

reference/shaders/legacy/vert/transpose.legacy.vert

@@ -0,0 +1,22 @@
+#version 100
+
+struct Buffer
+{
+    mat4 MVPRowMajor;
+    mat4 MVPColMajor;
+    mat4 M;
+};
+
+uniform Buffer _13;
+
+attribute vec4 Position;
+
+void main()
+{
+    vec4 c0 = _13.M * (Position * _13.MVPRowMajor);
+    vec4 c1 = _13.M * (_13.MVPColMajor * Position);
+    vec4 c2 = _13.M * (_13.MVPRowMajor * Position);
+    vec4 c3 = _13.M * (Position * _13.MVPColMajor);
+    gl_Position = ((c0 + c1) + c2) + c3;
+}
+

shaders/legacy/vert/transpose.legacy.vert

@@ -0,0 +1,20 @@
+#version 310 es
+
+uniform Buffer
+{
+	layout(row_major) mat4 MVPRowMajor;
+	layout(column_major) mat4 MVPColMajor;
+	mat4 M;
+};
+
+layout(location = 0) in vec4 Position;
+
+void main()
+{
+	vec4 c0 = M * (MVPRowMajor * Position);
+	vec4 c1 = M * (MVPColMajor * Position);
+	vec4 c2 = M * (Position * MVPRowMajor);
+	vec4 c3 = M * (Position * MVPColMajor);
+	gl_Position = c0 + c1 + c2 + c3;
+}
+

spirv_common.hpp

@@ -105,7 +105,7 @@ inline std::string convert_to_string(T &&t)
 
 #ifdef _MSC_VER
 #pragma warning(push)
-#pragma warning(disable: 4996)
+#pragma warning(disable : 4996)
 #endif
 
 inline std::string convert_to_string(float t)
@@ -351,6 +351,10 @@ struct SPIRExpression : IVariant
 	// If this expression has been used while invalidated.
 	bool used_while_invalidated = false;
 
+	// Before use, this expression must be transposed.
+	// This is needed for targets which don't support row_major layouts.
+	bool need_transpose = false;
+
 	// A list of expressions which this expression depends on.
 	std::vector<uint32_t> expression_dependencies;
 };

spirv_glsl.cpp

@@ -1000,12 +1000,36 @@ void CompilerGLSL::emit_push_constant_block_glsl(const SPIRVariable &var)
 	statement("");
 }
 
+void CompilerGLSL::emit_buffer_block_legacy(const SPIRVariable &var)
+{
+	auto &type = get<SPIRType>(var.basetype);
+
+	// We're emitting the push constant block as a regular struct, so disable the block qualifier temporarily.
+	// Otherwise, we will end up emitting layout() qualifiers on naked structs which is not allowed.
+	auto &block_flags = meta[type.self].decoration.decoration_flags;
+	uint64_t block_flag = block_flags & (1ull << DecorationBlock);
+	block_flags &= ~block_flag;
+	emit_struct(type);
+	block_flags |= block_flag;
+	emit_uniform(var);
+	statement("");
+}
+
 void CompilerGLSL::emit_buffer_block(const SPIRVariable &var)
 {
 	auto &type = get<SPIRType>(var.basetype);
 	bool ssbo = (meta[type.self].decoration.decoration_flags & (1ull << DecorationBufferBlock)) != 0;
 	bool is_restrict = (meta[var.self].decoration.decoration_flags & (1ull << DecorationRestrict)) != 0;
 
+	// By default, for legacy targets, fall back to declaring a uniform struct.
+	if (is_legacy())
+	{
+		if (ssbo)
+			SPIRV_CROSS_THROW("SSBOs not supported in legacy targets.");
+		emit_buffer_block_legacy(var);
+		return;
+	}
+
 	add_resource_name(var.self);
 
 	// Block names should never alias.
@@ -1055,6 +1079,9 @@ void CompilerGLSL::emit_interface_block(const SPIRVariable &var)
 
 	if (block)
 	{
+		if (is_legacy())
+			SPIRV_CROSS_THROW("IO blocks are not supported in legacy targets.");
+
 		add_resource_name(var.self);
 
 		// Block names should never alias.
@@ -1574,6 +1601,8 @@ string CompilerGLSL::to_expression(uint32_t id)
 		auto &e = get<SPIRExpression>(id);
 		if (e.base_expression)
 			return to_enclosed_expression(e.base_expression) + e.expression;
+		else if (e.need_transpose)
+			return convert_row_major_matrix(e.expression);
 		else
 			return e.expression;
 	}
@@ -2549,8 +2578,8 @@ void CompilerGLSL::emit_texture_op(const Instruction &i)
 
 	string expr;
 	bool forward = false;
-	expr += to_function_name(img, imgtype, !!fetch, !!gather, !!proj, !!coffsets, (!!coffset || !!offset), (!!grad_x || !!grad_y), !!lod,
-	                         !!dref);
+	expr += to_function_name(img, imgtype, !!fetch, !!gather, !!proj, !!coffsets, (!!coffset || !!offset),
+	                         (!!grad_x || !!grad_y), !!lod, !!dref);
 	expr += "(";
 	expr += to_function_args(img, imgtype, fetch, gather, proj, coord, coord_components, dref, grad_x, grad_y, lod,
 	                         coffset, offset, bias, comp, sample, &forward);
@@ -3097,7 +3126,7 @@ const char *CompilerGLSL::index_to_swizzle(uint32_t index)
 }
 
 string CompilerGLSL::access_chain(uint32_t base, const uint32_t *indices, uint32_t count, bool index_is_literal,
-                                  bool chain_only)
+                                  bool chain_only, bool *need_transpose)
 {
 	string expr;
 	if (!chain_only)
@@ -3218,6 +3247,8 @@ string CompilerGLSL::access_chain(uint32_t base, const uint32_t *indices, uint32
 			SPIRV_CROSS_THROW("Cannot subdivide a scalar value!");
 	}
 
+	if (need_transpose)
+		*need_transpose = row_major_matrix_needs_conversion;
 	return expr;
 }
 
@@ -3554,13 +3585,28 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 		// 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);
 
-		// If loading a non-native row-major matrix, convert it to column-major
+		// If loading a non-native row-major matrix, mark the expression as need_transpose.
+		bool need_transpose = false;
+		bool old_need_transpose = false;
+
+		auto *ptr_expression = maybe_get<SPIRExpression>(ptr);
+		if (ptr_expression && ptr_expression->need_transpose)
+		{
+			old_need_transpose = true;
+			ptr_expression->need_transpose = false;
+			need_transpose = true;
+		}
+		else if (is_non_native_row_major_matrix(ptr))
+			need_transpose = true;
+
 		auto expr = to_expression(ptr);
-		if (is_non_native_row_major_matrix(ptr))
-			expr = convert_row_major_matrix(expr);
+
+		if (ptr_expression)
+			ptr_expression->need_transpose = old_need_transpose;
 
 		// Suppress usage tracking since using same expression multiple times does not imply any extra work.
-		emit_op(result_type, id, expr, forward, true);
+		auto &e = emit_op(result_type, id, expr, forward, true);
+		e.need_transpose = need_transpose;
 		register_read(id, ptr, forward);
 		break;
 	}
@@ -3574,9 +3620,11 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 
 		// 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);
+		bool need_transpose;
+		auto e = access_chain(ops[2], &ops[3], length - 3, false, false, &need_transpose);
 		auto &expr = set<SPIRExpression>(ops[1], move(e), ops[0], should_forward(ops[2]));
 		expr.loaded_from = ops[2];
+		expr.need_transpose = need_transpose;
 		break;
 	}
 
@@ -4012,11 +4060,25 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 		break;
 	}
 
-	case OpFMul:
+	case OpVectorTimesMatrix:
 	case OpMatrixTimesVector:
+	{
+		// If the matrix needs transpose, just flip the multiply order.
+		auto *e = maybe_get<SPIRExpression>(ops[opcode == OpMatrixTimesVector ? 2 : 3]);
+		if (e && e->need_transpose)
+		{
+			e->need_transpose = false;
+			emit_binary_op(ops[0], ops[1], ops[3], ops[2], "*");
+			e->need_transpose = true;
+		}
+		else
+			BOP(*);
+		break;
+	}
+
+	case OpFMul:
 	case OpMatrixTimesScalar:
 	case OpVectorTimesScalar:
-	case OpVectorTimesMatrix:
 	case OpMatrixTimesMatrix:
 		BOP(*);
 		break;
@@ -4855,7 +4917,8 @@ void CompilerGLSL::add_member_name(SPIRType &type, uint32_t index)
 bool CompilerGLSL::is_non_native_row_major_matrix(uint32_t id)
 {
 	// Natively supported row-major matrices do not need to be converted.
-	if (backend.native_row_major_matrix)
+	// Legacy targets do not support row major.
+	if (backend.native_row_major_matrix && !is_legacy())
 		return false;
 
 	// Non-matrix or column-major matrix types do not need to be converted.
@@ -4876,7 +4939,7 @@ bool CompilerGLSL::is_non_native_row_major_matrix(uint32_t id)
 bool CompilerGLSL::member_is_non_native_row_major_matrix(const SPIRType &type, uint32_t index)
 {
 	// Natively supported row-major matrices do not need to be converted.
-	if (backend.native_row_major_matrix)
+	if (backend.native_row_major_matrix && !is_legacy())
 		return false;
 
 	// Non-matrix or column-major matrix types do not need to be converted.

spirv_glsl.hpp

@@ -264,6 +264,7 @@ protected:
 	void emit_struct(SPIRType &type);
 	void emit_resources();
 	void emit_buffer_block(const SPIRVariable &type);
+	void emit_buffer_block_legacy(const SPIRVariable &var);
 	void emit_push_constant_block(const SPIRVariable &var);
 	void emit_push_constant_block_vulkan(const SPIRVariable &var);
 	void emit_push_constant_block_glsl(const SPIRVariable &var);
@@ -305,7 +306,7 @@ protected:
 	SPIRExpression &emit_op(uint32_t result_type, uint32_t result_id, const std::string &rhs, bool forward_rhs,
 	                        bool suppress_usage_tracking = false);
 	std::string access_chain(uint32_t base, const uint32_t *indices, uint32_t count, bool index_is_literal,
-	                         bool chain_only = false);
+	                         bool chain_only = false, bool *need_transpose = nullptr);
 
 	const char *index_to_swizzle(uint32_t index);
 	std::string remap_swizzle(uint32_t result_type, uint32_t input_components, uint32_t expr);

test_shaders.py

@@ -66,7 +66,7 @@ def validate_shader(shader, vulkan):
     else:
         subprocess.check_call(['glslangValidator', shader])
 
-def cross_compile(shader, vulkan, spirv, eliminate, invalid_spirv):
+def cross_compile(shader, vulkan, spirv, eliminate, invalid_spirv, is_legacy):
     spirv_f, spirv_path = tempfile.mkstemp()
     glsl_f, glsl_path = tempfile.mkstemp(suffix = os.path.basename(shader))
     os.close(spirv_f)
@@ -84,11 +84,15 @@ def cross_compile(shader, vulkan, spirv, eliminate, invalid_spirv):
     if not invalid_spirv:
         subprocess.check_call(['spirv-val', spirv_path])
 
+    legacy_cmd = []
+    if is_legacy:
+        legacy_cmd = ['--version', '100', '--es']
+
     spirv_cross_path = './spirv-cross'
     if eliminate:
-        subprocess.check_call([spirv_cross_path, '--remove-unused-variables', '--entry', 'main', '--output', glsl_path, spirv_path])
+        subprocess.check_call([spirv_cross_path, '--remove-unused-variables', '--entry', 'main', '--output', glsl_path, spirv_path] + legacy_cmd)
     else:
-        subprocess.check_call([spirv_cross_path, '--entry', 'main', '--output', glsl_path, spirv_path])
+        subprocess.check_call([spirv_cross_path, '--entry', 'main', '--output', glsl_path, spirv_path] + legacy_cmd)
 
     # A shader might not be possible to make valid GLSL from, skip validation for this case.
     if (not ('nocompat' in glsl_path)) and (not spirv):
@@ -171,6 +175,9 @@ def shader_is_spirv(shader):
 def shader_is_invalid_spirv(shader):
     return '.invalid.' in shader
 
+def shader_is_legacy(shader):
+    return '.legacy.' in shader
+
 def test_shader(stats, shader, update, keep):
     joined_path = os.path.join(shader[0], shader[1])
     vulkan = shader_is_vulkan(shader[1])
@@ -178,9 +185,10 @@ def test_shader(stats, shader, update, keep):
     eliminate = shader_is_eliminate_dead_variables(shader[1])
     is_spirv = shader_is_spirv(shader[1])
     invalid_spirv = shader_is_invalid_spirv(shader[1])
+    is_legacy = shader_is_legacy(shader[1])
 
     print('Testing shader:', joined_path)
-    spirv, glsl, vulkan_glsl = cross_compile(joined_path, vulkan, is_spirv, eliminate, invalid_spirv)
+    spirv, glsl, vulkan_glsl = cross_compile(joined_path, vulkan, is_spirv, eliminate, invalid_spirv, is_legacy)
 
     # Only test GLSL stats if we have a shader following GL semantics.
     if stats and (not vulkan) and (not is_spirv) and (not desktop):