Merge pull request #655 from KhronosGroup/fix-643

MSL: Properly support passing parameters by value.

reference/opt/shaders-hlsl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,24 @@
+cbuffer registers
+{
+    float registers_foo : packoffset(c0);
+};
+
+static float FragColor;
+
+struct SPIRV_Cross_Output
+{
+    float FragColor : SV_Target0;
+};
+
+void frag_main()
+{
+    FragColor = 10.0f + registers_foo;
+}
+
+SPIRV_Cross_Output main()
+{
+    frag_main();
+    SPIRV_Cross_Output stage_output;
+    stage_output.FragColor = FragColor;
+    return stage_output;
+}

reference/opt/shaders-msl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,22 @@
+#include <metal_stdlib>
+#include <simd/simd.h>
+
+using namespace metal;
+
+struct Registers
+{
+    float foo;
+};
+
+struct main0_out
+{
+    float FragColor [[color(0)]];
+};
+
+fragment main0_out main0(constant Registers& registers [[buffer(0)]])
+{
+    main0_out out = {};
+    out.FragColor = 10.0 + registers.foo;
+    return out;
+}
+

reference/opt/shaders/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,16 @@
+#version 450
+
+struct Registers
+{
+    float foo;
+};
+
+uniform Registers registers;
+
+layout(location = 0) out float FragColor;
+
+void main()
+{
+    FragColor = 10.0 + registers.foo;
+}
+

reference/shaders-hlsl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,29 @@
+cbuffer registers
+{
+    float registers_foo : packoffset(c0);
+};
+
+static float FragColor;
+
+struct SPIRV_Cross_Output
+{
+    float FragColor : SV_Target0;
+};
+
+float add_value(float v, float w)
+{
+    return v + w;
+}
+
+void frag_main()
+{
+    FragColor = add_value(10.0f, registers_foo);
+}
+
+SPIRV_Cross_Output main()
+{
+    frag_main();
+    SPIRV_Cross_Output stage_output;
+    stage_output.FragColor = FragColor;
+    return stage_output;
+}

reference/shaders-msl/asm/frag/empty-struct.asm.frag

@@ -15,7 +15,7 @@ float GetValue(thread const EmptyStructTest& self)
     return 0.0;
 }
 
-float GetValue_1(thread const EmptyStructTest& self)
+float GetValue_1(EmptyStructTest self)
 {
     return 0.0;
 }

reference/shaders-msl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,29 @@
+#pragma clang diagnostic ignored "-Wmissing-prototypes"
+
+#include <metal_stdlib>
+#include <simd/simd.h>
+
+using namespace metal;
+
+struct Registers
+{
+    float foo;
+};
+
+struct main0_out
+{
+    float FragColor [[color(0)]];
+};
+
+float add_value(float v, float w)
+{
+    return v + w;
+}
+
+fragment main0_out main0(constant Registers& registers [[buffer(0)]])
+{
+    main0_out out = {};
+    out.FragColor = add_value(10.0, registers.foo);
+    return out;
+}
+

reference/shaders/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,21 @@
+#version 450
+
+struct Registers
+{
+    float foo;
+};
+
+uniform Registers registers;
+
+layout(location = 0) out float FragColor;
+
+float add_value(float v, float w)
+{
+    return v + w;
+}
+
+void main()
+{
+    FragColor = add_value(10.0, registers.foo);
+}
+

shaders-hlsl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,51 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 6
+; Bound: 32
+; Schema: 0
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %FragColor
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 450
+               OpName %main "main"
+               OpName %add_value_f1_f1_ "add_value(f1;f1;"
+               OpName %v "v"
+               OpName %w "w"
+               OpName %FragColor "FragColor"
+               OpName %Registers "Registers"
+               OpMemberName %Registers 0 "foo"
+               OpName %registers "registers"
+               OpDecorate %FragColor Location 0
+               OpMemberDecorate %Registers 0 Offset 0
+               OpDecorate %Registers Block
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+%_ptr_Function_float = OpTypePointer Function %float
+          %8 = OpTypeFunction %float %float %float
+%_ptr_Output_float = OpTypePointer Output %float
+  %FragColor = OpVariable %_ptr_Output_float Output
+   %float_10 = OpConstant %float 10
+  %Registers = OpTypeStruct %float
+%_ptr_PushConstant_Registers = OpTypePointer PushConstant %Registers
+  %registers = OpVariable %_ptr_PushConstant_Registers PushConstant
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+%_ptr_PushConstant_float = OpTypePointer PushConstant %float
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %29 = OpAccessChain %_ptr_PushConstant_float %registers %int_0
+         %30 = OpLoad %float %29
+         %31 = OpFunctionCall %float %add_value_f1_f1_ %float_10 %30
+               OpStore %FragColor %31
+               OpReturn
+               OpFunctionEnd
+%add_value_f1_f1_ = OpFunction %float None %8
+          %v = OpFunctionParameter %float
+          %w = OpFunctionParameter %float
+         %12 = OpLabel
+         %15 = OpFAdd %float %v %w
+               OpReturnValue %15
+               OpFunctionEnd

shaders-msl/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,51 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 6
+; Bound: 32
+; Schema: 0
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %FragColor
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 450
+               OpName %main "main"
+               OpName %add_value_f1_f1_ "add_value(f1;f1;"
+               OpName %v "v"
+               OpName %w "w"
+               OpName %FragColor "FragColor"
+               OpName %Registers "Registers"
+               OpMemberName %Registers 0 "foo"
+               OpName %registers "registers"
+               OpDecorate %FragColor Location 0
+               OpMemberDecorate %Registers 0 Offset 0
+               OpDecorate %Registers Block
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+%_ptr_Function_float = OpTypePointer Function %float
+          %8 = OpTypeFunction %float %float %float
+%_ptr_Output_float = OpTypePointer Output %float
+  %FragColor = OpVariable %_ptr_Output_float Output
+   %float_10 = OpConstant %float 10
+  %Registers = OpTypeStruct %float
+%_ptr_PushConstant_Registers = OpTypePointer PushConstant %Registers
+  %registers = OpVariable %_ptr_PushConstant_Registers PushConstant
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+%_ptr_PushConstant_float = OpTypePointer PushConstant %float
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %29 = OpAccessChain %_ptr_PushConstant_float %registers %int_0
+         %30 = OpLoad %float %29
+         %31 = OpFunctionCall %float %add_value_f1_f1_ %float_10 %30
+               OpStore %FragColor %31
+               OpReturn
+               OpFunctionEnd
+%add_value_f1_f1_ = OpFunction %float None %8
+          %v = OpFunctionParameter %float
+          %w = OpFunctionParameter %float
+         %12 = OpLabel
+         %15 = OpFAdd %float %v %w
+               OpReturnValue %15
+               OpFunctionEnd

shaders/asm/frag/pass-by-value.asm.frag

@@ -0,0 +1,51 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 6
+; Bound: 32
+; Schema: 0
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %FragColor
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 450
+               OpName %main "main"
+               OpName %add_value_f1_f1_ "add_value(f1;f1;"
+               OpName %v "v"
+               OpName %w "w"
+               OpName %FragColor "FragColor"
+               OpName %Registers "Registers"
+               OpMemberName %Registers 0 "foo"
+               OpName %registers "registers"
+               OpDecorate %FragColor Location 0
+               OpMemberDecorate %Registers 0 Offset 0
+               OpDecorate %Registers Block
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+%_ptr_Function_float = OpTypePointer Function %float
+          %8 = OpTypeFunction %float %float %float
+%_ptr_Output_float = OpTypePointer Output %float
+  %FragColor = OpVariable %_ptr_Output_float Output
+   %float_10 = OpConstant %float 10
+  %Registers = OpTypeStruct %float
+%_ptr_PushConstant_Registers = OpTypePointer PushConstant %Registers
+  %registers = OpVariable %_ptr_PushConstant_Registers PushConstant
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+%_ptr_PushConstant_float = OpTypePointer PushConstant %float
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %29 = OpAccessChain %_ptr_PushConstant_float %registers %int_0
+         %30 = OpLoad %float %29
+         %31 = OpFunctionCall %float %add_value_f1_f1_ %float_10 %30
+               OpStore %FragColor %31
+               OpReturn
+               OpFunctionEnd
+%add_value_f1_f1_ = OpFunction %float None %8
+          %v = OpFunctionParameter %float
+          %w = OpFunctionParameter %float
+         %12 = OpLabel
+         %15 = OpFAdd %float %v %w
+               OpReturnValue %15
+               OpFunctionEnd

spirv_msl.cpp

@@ -565,10 +565,20 @@ void CompilerMSL::extract_global_variables_from_function(uint32_t func_id, std::
 					if (is_builtin && has_active_builtin(builtin, var.storage))
 					{
 						// Add a arg variable with the same type and decorations as the member
-						uint32_t next_id = increase_bound_by(1);
+						uint32_t next_ids = increase_bound_by(2);
-						func.add_parameter(mbr_type_id, next_id, true);
+						uint32_t ptr_type_id = next_ids + 0;
-						set<SPIRVariable>(next_id, mbr_type_id, StorageClassFunction);
+						uint32_t var_id = next_ids + 1;
-						meta[next_id].decoration = meta[type_id].members[mbr_idx];
+
+						// Make sure we have an actual pointer type,
+						// so that we will get the appropriate address space when declaring these builtins.
+						auto &ptr = set<SPIRType>(ptr_type_id, get<SPIRType>(mbr_type_id));
+						ptr.self = mbr_type_id;
+						ptr.storage = var.storage;
+						ptr.pointer = true;
+
+						func.add_parameter(mbr_type_id, var_id, true);
+						set<SPIRVariable>(var_id, ptr_type_id, StorageClassFunction);
+						meta[var_id].decoration = meta[type_id].members[mbr_idx];
 					}
 					mbr_idx++;
 				}
@@ -2400,8 +2410,7 @@ void CompilerMSL::emit_function_prototype(SPIRFunction &func, const Bitset &)
 	{
 		add_local_variable_name(arg.id);
 
-		string address_space = "thread";
+		string address_space;
-
 		auto *var = maybe_get<SPIRVariable>(arg.id);
 		if (var)
 		{
@@ -2409,7 +2418,8 @@ void CompilerMSL::emit_function_prototype(SPIRFunction &func, const Bitset &)
 			address_space = get_argument_address_space(*var);
 		}
 
-		decl += address_space + " ";
+		if (!address_space.empty())
+			decl += address_space + " ";
 		decl += argument_decl(arg);
 
 		// Manufacture automatic sampler arg for SampledImage texture
@@ -3161,6 +3171,11 @@ string CompilerMSL::get_argument_address_space(const SPIRVariable &argument)
 		}
 		break;
 
+	case StorageClassFunction:
+	case StorageClassGeneric:
+		// No address space for plain values.
+		return type.pointer ? "thread" : "";
+
 	default:
 		break;
 	}
@@ -3374,7 +3389,8 @@ string CompilerMSL::argument_decl(const SPIRFunction::Parameter &arg)
 {
 	auto &var = get<SPIRVariable>(arg.id);
 	auto &type = expression_type(arg.id);
-	bool constref = !arg.alias_global_variable && (!type.pointer || arg.write_count == 0);
+
+	bool constref = !arg.alias_global_variable && type.pointer && arg.write_count == 0;
 
 	bool type_is_image = type.basetype == SPIRType::Image || type.basetype == SPIRType::SampledImage ||
 	                     type.basetype == SPIRType::Sampler;
@@ -3383,27 +3399,34 @@ string CompilerMSL::argument_decl(const SPIRFunction::Parameter &arg)
 	if (!type.array.empty() && type_is_image)
 		constref = true;
 
-	// TODO: Check if this arg is an uniform pointer
-	bool pointer = type.storage == StorageClassUniformConstant;
-
 	string decl;
 	if (constref)
 		decl += "const ";
 
-	if (is_builtin_variable(var))
+	bool builtin = is_builtin_variable(var);
+	if (builtin)
 		decl += builtin_type_decl(static_cast<BuiltIn>(get_decoration(arg.id, DecorationBuiltIn)));
 	else
 		decl += type_to_glsl(type, arg.id);
 
-	// Arrays of images and samplers are special cased.
+	bool opaque_handle = type.storage == StorageClassUniformConstant;
-	if (is_array(type) && !type_is_image)
+
+	if (!builtin && !opaque_handle && !type.pointer &&
+	    (type.storage == StorageClassFunction || type.storage == StorageClassGeneric))
 	{
+		// If the argument is a pure value and not an opaque type, we will pass by value.
+		decl += " ";
+		decl += to_expression(var.self);
+	}
+	else if (is_array(type) && !type_is_image)
+	{
+		// Arrays of images and samplers are special cased.
 		decl += " (&";
 		decl += to_expression(var.self);
 		decl += ")";
 		decl += type_to_array_glsl(type);
 	}
-	else if (!pointer)
+	else if (!opaque_handle)
 	{
 		decl += "&";
 		decl += " ";