Merge pull request #1863 from KhronosGroup/various-fixes

Various fixes

main.cpp

@@ -669,6 +669,7 @@ struct CLIArguments
 	bool emit_line_directives = false;
 	bool enable_storage_image_qualifier_deduction = true;
 	bool force_zero_initialized_variables = false;
+	uint32_t force_recompile_max_debug_iterations = 3;
 	SmallVector<uint32_t> msl_discrete_descriptor_sets;
 	SmallVector<uint32_t> msl_device_argument_buffers;
 	SmallVector<pair<uint32_t, uint32_t>> msl_dynamic_buffers;
@@ -932,6 +933,8 @@ static void print_help_obscure()
 	                "\t\tdo not attempt to analyze usage, and always emit read/write state.\n"
 	                "\t[--flatten-multidimensional-arrays]:\n\t\tDo not support multi-dimensional arrays and flatten them to one dimension.\n"
 	                "\t[--cpp-interface-name <name>]:\n\t\tEmit a specific class name in C++ codegen.\n"
+	                "\t[--force-recompile-max-debug-iterations <count>]:\n\t\tAllow compilation loop to run for N loops.\n"
+	                "\t\tCan be used to triage workarounds, but should not be used as a crutch, since it masks an implementation bug.\n"
 	);
 	// clang-format on
 }
@@ -1286,6 +1289,7 @@ static string compile_iteration(const CLIArguments &args, std::vector<uint32_t>
 	opts.emit_line_directives = args.emit_line_directives;
 	opts.enable_storage_image_qualifier_deduction = args.enable_storage_image_qualifier_deduction;
 	opts.force_zero_initialized_variables = args.force_zero_initialized_variables;
+	opts.force_recompile_max_debug_iterations = args.force_recompile_max_debug_iterations;
 	compiler->set_common_options(opts);
 
 	for (auto &fetch : args.glsl_ext_framebuffer_fetch)
@@ -1678,6 +1682,10 @@ static int main_inner(int argc, char *argv[])
 		args.masked_stage_builtins.push_back(masked_builtin);
 	});
 
+	cbs.add("--force-recompile-max-debug-iterations", [&](CLIParser &parser) {
+		args.force_recompile_max_debug_iterations = parser.next_uint();
+	});
+
 	cbs.default_handler = [&args](const char *value) { args.input = value; };
 	cbs.add("-", [&args](CLIParser &) { args.input = "-"; });
 	cbs.error_handler = [] { print_help(); };

reference/shaders-no-opt/asm/frag/texel-fetch-ms-uint-sample.asm.frag

@@ -0,0 +1,15 @@
+#version 450
+
+layout(binding = 0) uniform sampler2DMS uSamp;
+
+layout(location = 0) out vec4 FragColor;
+
+void main()
+{
+    ivec2 _28 = ivec2(gl_FragCoord.xy);
+    FragColor.x = texelFetch(uSamp, _28, int(0u)).x;
+    FragColor.y = texelFetch(uSamp, _28, int(1u)).x;
+    FragColor.z = texelFetch(uSamp, _28, int(2u)).x;
+    FragColor.w = texelFetch(uSamp, _28, int(3u)).x;
+}
+

shaders-no-opt/asm/frag/texel-fetch-ms-uint-sample.asm.frag

@@ -0,0 +1,68 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 10
+; Bound: 61
+; Schema: 0
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %FragColor %gl_FragCoord
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 450
+               OpName %main "main"
+               OpName %FragColor "FragColor"
+               OpName %uSamp "uSamp"
+               OpName %gl_FragCoord "gl_FragCoord"
+               OpDecorate %FragColor Location 0
+               OpDecorate %uSamp DescriptorSet 0
+               OpDecorate %uSamp Binding 0
+               OpDecorate %gl_FragCoord BuiltIn FragCoord
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+  %FragColor = OpVariable %_ptr_Output_v4float Output
+         %10 = OpTypeImage %float 2D 0 0 1 1 Unknown
+         %11 = OpTypeSampledImage %10
+%_ptr_UniformConstant_11 = OpTypePointer UniformConstant %11
+      %uSamp = OpVariable %_ptr_UniformConstant_11 UniformConstant
+%_ptr_Input_v4float = OpTypePointer Input %v4float
+%gl_FragCoord = OpVariable %_ptr_Input_v4float Input
+    %v2float = OpTypeVector %float 2
+       %uint = OpTypeInt 32 0
+       %int = OpTypeInt 32 1
+	   %v2int = OpTypeVector %int 2
+     %uint_0 = OpConstant %uint 0
+%_ptr_Output_float = OpTypePointer Output %float
+     %uint_1 = OpConstant %uint 1
+     %uint_2 = OpConstant %uint 2
+     %uint_3 = OpConstant %uint 3
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %14 = OpLoad %11 %uSamp
+         %18 = OpLoad %v4float %gl_FragCoord
+         %19 = OpVectorShuffle %v2float %18 %18 0 1
+         %22 = OpConvertFToS %v2int %19
+         %24 = OpImage %10 %14
+         %25 = OpImageFetch %v4float %24 %22 Sample %uint_0
+         %28 = OpCompositeExtract %float %25 0
+         %30 = OpAccessChain %_ptr_Output_float %FragColor %uint_0
+               OpStore %30 %28
+         %36 = OpImage %10 %14
+         %37 = OpImageFetch %v4float %36 %22 Sample %uint_1
+         %38 = OpCompositeExtract %float %37 0
+         %40 = OpAccessChain %_ptr_Output_float %FragColor %uint_1
+               OpStore %40 %38
+         %46 = OpImage %10 %14
+         %47 = OpImageFetch %v4float %46 %22 Sample %uint_2
+         %48 = OpCompositeExtract %float %47 0
+         %50 = OpAccessChain %_ptr_Output_float %FragColor %uint_2
+               OpStore %50 %48
+         %56 = OpImage %10 %14
+         %57 = OpImageFetch %v4float %56 %22 Sample %uint_3
+         %58 = OpCompositeExtract %float %57 0
+         %60 = OpAccessChain %_ptr_Output_float %FragColor %uint_3
+               OpStore %60 %58
+               OpReturn
+               OpFunctionEnd

spirv_cross.cpp

@@ -1677,6 +1677,11 @@ const SmallVector<SPIRBlock::Case> &Compiler::get_case_list(const SPIRBlock &blo
 		const auto &type = get<SPIRType>(var->basetype);
 		width = type.width;
 	}
+	else if (const auto *undef = maybe_get<SPIRUndef>(block.condition))
+	{
+		const auto &type = get<SPIRType>(undef->basetype);
+		width = type.width;
+	}
 	else
 	{
 		auto search = ir.load_type_width.find(block.condition);

spirv_glsl.cpp

@@ -306,8 +306,8 @@ void CompilerGLSL::reset(uint32_t iteration_count)
 	// It is highly context-sensitive when we need to force recompilation,
 	// and it is not practical with the current architecture
 	// to resolve everything up front.
-	if (iteration_count >= 3 && !is_force_recompile_forward_progress)
-		SPIRV_CROSS_THROW("Over 3 compilation loops detected and no forward progress was made. Must be a bug!");
+	if (iteration_count >= options.force_recompile_max_debug_iterations && !is_force_recompile_forward_progress)
+		SPIRV_CROSS_THROW("Maximum compilation loops detected and no forward progress was made. Must be a SPIRV-Cross bug!");
 
 	// We do some speculative optimizations which should pretty much always work out,
 	// but just in case the SPIR-V is rather weird, recompile until it's happy.
@@ -4298,10 +4298,8 @@ string CompilerGLSL::to_func_call_arg(const SPIRFunction::Parameter &, uint32_t
 	return to_expression(name_id);
 }
 
-void CompilerGLSL::handle_invalid_expression(uint32_t id)
+void CompilerGLSL::force_temporary_and_recompile(uint32_t id)
 {
-	// 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.
 	auto res = forced_temporaries.insert(id);
 
 	// Forcing new temporaries guarantees forward progress.
@@ -4311,6 +4309,14 @@ void CompilerGLSL::handle_invalid_expression(uint32_t id)
 		force_recompile();
 }
 
+void CompilerGLSL::handle_invalid_expression(uint32_t id)
+{
+	// 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.
+	force_temporary_and_recompile(id);
+}
+
 // Converts the format of the current expression from packed to unpacked,
 // by wrapping the expression in a constructor of the appropriate type.
 // GLSL does not support packed formats, so simply return the expression.
@@ -7237,18 +7243,11 @@ string CompilerGLSL::to_function_args(const TextureFunctionArguments &args, bool
 			forward = forward && should_forward(args.lod);
 			farg_str += ", ";
 
-			auto &lod_expr_type = expression_type(args.lod);
-
 			// Lod expression for TexelFetch in GLSL must be int, and only int.
-			if (args.base.is_fetch && imgtype.image.dim != DimBuffer && !imgtype.image.ms &&
-			    lod_expr_type.basetype != SPIRType::Int)
-			{
-				farg_str += join("int(", to_expression(args.lod), ")");
-			}
+			if (args.base.is_fetch && imgtype.image.dim != DimBuffer && !imgtype.image.ms)
+				farg_str += bitcast_expression(SPIRType::Int, args.lod);
 			else
-			{
 				farg_str += to_expression(args.lod);
-			}
 		}
 	}
 	else if (args.base.is_fetch && imgtype.image.dim != DimBuffer && !imgtype.image.ms)
@@ -7261,19 +7260,19 @@ string CompilerGLSL::to_function_args(const TextureFunctionArguments &args, bool
 	{
 		forward = forward && should_forward(args.coffset);
 		farg_str += ", ";
-		farg_str += to_expression(args.coffset);
+		farg_str += bitcast_expression(SPIRType::Int, args.coffset);
 	}
 	else if (args.offset)
 	{
 		forward = forward && should_forward(args.offset);
 		farg_str += ", ";
-		farg_str += to_expression(args.offset);
+		farg_str += bitcast_expression(SPIRType::Int, args.offset);
 	}
 
 	if (args.sample)
 	{
 		farg_str += ", ";
-		farg_str += to_expression(args.sample);
+		farg_str += bitcast_expression(SPIRType::Int, args.sample);
 	}
 
 	if (args.min_lod)
@@ -7300,11 +7299,7 @@ string CompilerGLSL::to_function_args(const TextureFunctionArguments &args, bool
 	{
 		forward = forward && should_forward(args.component);
 		farg_str += ", ";
-		auto &component_type = expression_type(args.component);
-		if (component_type.basetype == SPIRType::Int)
-			farg_str += to_expression(args.component);
-		else
-			farg_str += join("int(", to_expression(args.component), ")");
+		farg_str += bitcast_expression(SPIRType::Int, args.component);
 	}
 
 	*p_forward = forward;
@@ -9596,9 +9591,8 @@ void CompilerGLSL::track_expression_read(uint32_t id)
 			//if (v == 2)
 			//    fprintf(stderr, "ID %u was forced to temporary due to more than 1 expression use!\n", id);
 
-			forced_temporaries.insert(id);
 			// Force a recompile after this pass to avoid forwarding this variable.
-			force_recompile();
+			force_temporary_and_recompile(id);
 		}
 	}
 }
@@ -9952,9 +9946,8 @@ void CompilerGLSL::disallow_forwarding_in_expression_chain(const SPIRExpression
 	if (expression_is_forwarded(expr.self) && !expression_suppresses_usage_tracking(expr.self) &&
 	    forced_invariant_temporaries.count(expr.self) == 0)
 	{
-		forced_temporaries.insert(expr.self);
+		force_temporary_and_recompile(expr.self);
 		forced_invariant_temporaries.insert(expr.self);
-		force_recompile();
 
 		for (auto &dependent : expr.expression_dependencies)
 			disallow_forwarding_in_expression_chain(get<SPIRExpression>(dependent));
@@ -14637,9 +14630,16 @@ void CompilerGLSL::emit_hoisted_temporaries(SmallVector<pair<TypeID, ID>> &tempo
 
 	for (auto &tmp : temporaries)
 	{
+		auto &type = get<SPIRType>(tmp.first);
+
+		// There are some rare scenarios where we are asked to declare pointer types as hoisted temporaries.
+		// This should be ignored unless we're doing actual variable pointers and backend supports it.
+		// Access chains cannot normally be lowered to temporaries in GLSL and HLSL.
+		if (type.pointer && !backend.native_pointers)
+			continue;
+
 		add_local_variable_name(tmp.second);
 		auto &flags = ir.meta[tmp.second].decoration.decoration_flags;
-		auto &type = get<SPIRType>(tmp.first);
 
 		// Not all targets support pointer literals, so don't bother with that case.
 		string initializer;

spirv_glsl.hpp

@@ -83,6 +83,11 @@ public:
 
 		// Debug option to always emit temporary variables for all expressions.
 		bool force_temporary = false;
+		// Debug option, can be increased in an attempt to workaround SPIRV-Cross bugs temporarily.
+		// If this limit has to be increased, it points to an implementation bug.
+		// In certain scenarios, the maximum number of debug iterations may increase beyond this limit
+		// as long as we can prove we're making certain kinds of forward progress.
+		uint32_t force_recompile_max_debug_iterations = 3;
 
 		// If true, Vulkan GLSL features are used instead of GL-compatible features.
 		// Mostly useful for debugging SPIR-V files.
@@ -881,6 +886,7 @@ protected:
 
 	void check_function_call_constraints(const uint32_t *args, uint32_t length);
 	void handle_invalid_expression(uint32_t id);
+	void force_temporary_and_recompile(uint32_t id);
 	void find_static_extensions();
 
 	std::string emit_for_loop_initializers(const SPIRBlock &block);

spirv_parser.cpp

@@ -291,7 +291,15 @@ void Parser::parse(const Instruction &instruction)
 	{
 		// The SPIR-V debug information extended instructions might come at global scope.
 		if (current_block)
+		{
 			current_block->ops.push_back(instruction);
+			if (length >= 2)
+			{
+				const auto *type = maybe_get<SPIRType>(ops[0]);
+				if (type)
+					ir.load_type_width.insert({ ops[1], type->width });
+			}
+		}
 		break;
 	}
 
@@ -1211,10 +1219,9 @@ void Parser::parse(const Instruction &instruction)
 		{
 			const auto *type = maybe_get<SPIRType>(ops[0]);
 			if (type)
-			{
 				ir.load_type_width.insert({ ops[1], type->width });
-			}
 		}
+
 		if (!current_block)
 			SPIRV_CROSS_THROW("Currently no block to insert opcode.");