GLSL: Consider hazards with load-store on buffer pointers.

Author: HansKristian-Work

reference/shaders-no-opt/asm/frag/descriptor-heap-hazard-tracking.vk.nocompat.asm.spv16.frag.vk

@@ -0,0 +1,26 @@
+#version 460
+#extension GL_EXT_descriptor_heap : require
+#extension GL_EXT_nonuniform_qualifier : require
+#extension GL_EXT_shader_image_load_formatted : require
+
+layout(location = 0) out float FragColor;
+layout(descriptor_heap, std430) readonly buffer SSBONoWrite
+{
+    vec4 data[];
+} spvSSBONoWriteResourceHeap[];
+
+layout(descriptor_heap, std430) buffer SSBO
+{
+    vec4 data[];
+} spvSSBOResourceHeap[];
+
+
+void main()
+{
+    spvSSBOResourceHeap[2].data[0].z = 0.0;
+    FragColor = spvSSBONoWriteResourceHeap[2].data[2].z;
+    float _34 = spvSSBOResourceHeap[2].data[0].z;
+    spvSSBOResourceHeap[2].data[0].z = 0.0;
+    FragColor = _34;
+}
+

shaders-no-opt/asm/frag/descriptor-heap-hazard-tracking.vk.nocompat.asm.spv16.frag

@@ -0,0 +1,76 @@
+; SPIR-V
+; Version: 1.0
+; Generator: Khronos Glslang Reference Front End; 11
+; Bound: 31
+; Schema: 0
+               OpCapability Shader
+               OpCapability UntypedPointersKHR
+               OpCapability DescriptorHeapEXT
+               OpExtension "SPV_EXT_descriptor_heap"
+               OpExtension "SPV_KHR_untyped_pointers"
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %FragColor %resource_heap
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 460
+               OpSourceExtension "GL_EXT_descriptor_heap"
+               OpSourceExtension "GL_EXT_nonuniform_qualifier"
+               OpName %main "main"
+               OpName %FragColor "FragColor"
+               OpName %resource_heap "resource_heap"
+               OpName %SSBO "SSBO"
+               OpMemberName %SSBO 0 "data"
+               OpDecorate %FragColor Location 0
+               OpDecorate %resource_heap BuiltIn ResourceHeapEXT
+               OpDecorate %_runtimearr_v4float ArrayStride 16
+               OpDecorate %SSBO Block
+			   OpDecorate %readonly NonWritable
+			   OpDecorate %writeonly Coherent
+               OpMemberDecorate %SSBO 0 Offset 0
+               OpDecorateId %_runtimearr_20 ArrayStrideIdEXT %21
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+%_ptr_Output_float = OpTypePointer Output %float
+  %FragColor = OpVariable %_ptr_Output_float Output
+    %float_0 = OpConstant %float 0
+%_ptr_UniformConstant = OpTypeUntypedPointerKHR UniformConstant
+%resource_heap = OpUntypedVariableKHR %_ptr_UniformConstant UniformConstant
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+      %int_2 = OpConstant %int 2
+%_runtimearr_v4float = OpTypeRuntimeArray %v4float
+       %SSBO = OpTypeStruct %_runtimearr_v4float
+%_ptr_StorageBuffer = OpTypeUntypedPointerKHR StorageBuffer
+%_ptr_StorageBuffer_SSBO = OpTypePointer StorageBuffer %SSBO
+%_ptr_StorageBuffer_v4float = OpTypePointer StorageBuffer %v4float
+%_ptr_StorageBuffer_float = OpTypePointer StorageBuffer %float
+         %20 = OpTypeBufferEXT StorageBuffer
+         %21 = OpConstantSizeOfEXT %int %20
+%_runtimearr_20 = OpTypeRuntimeArray %20
+       %uint = OpTypeInt 32 0
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %19 = OpUntypedAccessChainKHR %_ptr_UniformConstant %_runtimearr_20 %resource_heap %int_2
+         %readonly = OpBufferPointerEXT %_ptr_StorageBuffer %19
+         %writeonly = OpBufferPointerEXT %_ptr_StorageBuffer %19
+
+		 %chain_load = OpUntypedAccessChainKHR %_ptr_StorageBuffer %SSBO %readonly %int_0 %int_2
+		 %chain_load_again = OpUntypedAccessChainKHR %_ptr_StorageBuffer %v4float %chain_load %int_2
+
+		 %chain_store = OpUntypedAccessChainKHR %_ptr_StorageBuffer %SSBO %writeonly %int_0 %int_0
+		 %chain_store_again = OpUntypedAccessChainKHR %_ptr_StorageBuffer %v4float %chain_store %int_2
+
+		; Read through different OpBufferPointerEXT. Aliasing is not considered.
+		 %load_readonly = OpLoad %float %chain_load_again
+		 OpStore %chain_store_again %float_0
+               OpStore %FragColor %load_readonly
+
+	    ; Now read on the same underlying OpBufferPointerEXT. We have to be careful.
+		%load_rw = OpLoad %float %chain_store_again
+		 OpStore %chain_store_again %float_0
+               OpStore %FragColor %load_rw
+
+               OpReturn
+               OpFunctionEnd

spirv_common.hpp

@@ -817,6 +817,10 @@ struct SPIRExpression : IVariant
 	// If this expression represents a OpBufferPointerEXT cast.
 	bool buffer_pointer = false;
 
+	// Temporaries which can remain forwarded as long as this variable is not modified.
+	// Only used for buffer pointers.
+	SmallVector<ID> buffer_pointer_dependees;
+
 	// A list of expressions which this expression depends on.
 	SmallVector<ID> expression_dependencies;
 

spirv_cross.cpp

@@ -434,7 +434,7 @@ SPIRVariable *Compiler::maybe_get_backing_variable(uint32_t chain)
 	return var;
 }
 
-const SPIRExpression *Compiler::maybe_get_backing_buffer_pointer(uint32_t chain) const
+SPIRExpression *Compiler::maybe_get_backing_buffer_pointer(uint32_t chain)
 {
 	auto *expr = maybe_get<SPIRExpression>(chain);
 	while (expr && !expr->buffer_pointer && expr->loaded_from)
@@ -446,6 +446,7 @@ void Compiler::register_read(uint32_t expr, uint32_t chain, bool forwarded)
 {
 	auto &e = get<SPIRExpression>(expr);
 	auto *var = maybe_get_backing_variable(chain);
+	auto *buffer_pointer = maybe_get_backing_buffer_pointer(chain);
 
 	if (var)
 	{
@@ -460,6 +461,13 @@ void Compiler::register_read(uint32_t expr, uint32_t chain, bool forwarded)
 		if (var && var->parameter)
 			var->parameter->read_count++;
 	}
+	else if (buffer_pointer)
+	{
+		e.loaded_from = buffer_pointer->self;
+		// If the backing variable is immutable, we do not need to depend on the variable.
+		if (forwarded && !is_immutable(buffer_pointer->self))
+			buffer_pointer->buffer_pointer_dependees.push_back(e.self);
+	}
 }
 
 void Compiler::register_write(uint32_t chain)
@@ -477,6 +485,8 @@ void Compiler::register_write(uint32_t chain)
 			var = maybe_get<SPIRVariable>(access_chain->loaded_from);
 	}
 
+	auto *buffer_pointer = maybe_get_backing_buffer_pointer(chain);
+
 	auto &chain_type = expression_type(chain);
 
 	if (var)
@@ -487,12 +497,7 @@ void Compiler::register_write(uint32_t chain)
 		// If our variable is in a storage class which can alias with other buffers,
 		// invalidate all variables which depend on aliased variables. And if this is a
 		// variable pointer, then invalidate all variables regardless.
-		if (BuiltIn(get_decoration(var->self, DecorationBuiltIn)) == BuiltInResourceHeapEXT)
-		{
-			// Very free form aliasing, be conservative.
-			flush_all_active_variables();
-		}
-		else if (get_variable_data_type(*var).pointer)
+		if (get_variable_data_type(*var).pointer)
 		{
 			flush_all_active_variables();
 
@@ -526,6 +531,10 @@ void Compiler::register_write(uint32_t chain)
 			force_recompile();
 		}
 	}
+	else if (buffer_pointer)
+	{
+		flush_dependees(*buffer_pointer);
+	}
 	else if (chain_type.pointer)
 	{
 		// If we stored through a variable pointer, then we don't know which
@@ -547,6 +556,16 @@ void Compiler::flush_dependees(SPIRVariable &var)
 	var.dependees.clear();
 }
 
+void Compiler::flush_dependees(SPIRExpression &expr)
+{
+	// A little ugly to split things up like this since BufferPointerEXT is a weird case
+	// where it's both an expression (chain into global heap) and a memory declaration at the same time ...
+	assert(expr.buffer_pointer);
+	for (auto dep : expr.buffer_pointer_dependees)
+		invalid_expressions.insert(dep);
+	expr.buffer_pointer_dependees.clear();
+}
+
 void Compiler::flush_all_aliased_variables()
 {
 	for (auto aliased : aliased_variables)
@@ -557,6 +576,8 @@ void Compiler::flush_all_atomic_capable_variables()
 {
 	for (auto global : global_variables)
 		flush_dependees(get<SPIRVariable>(global));
+	for (auto global : buffer_pointer_variables)
+		flush_dependees(get<SPIRExpression>(global));
 	flush_all_aliased_variables();
 }
 
@@ -578,6 +599,8 @@ void Compiler::flush_all_active_variables()
 		flush_dependees(get<SPIRVariable>(arg.id));
 	for (auto global : global_variables)
 		flush_dependees(get<SPIRVariable>(global));
+	for (auto global : buffer_pointer_variables)
+		flush_dependees(get<SPIRExpression>(global));
 
 	flush_all_aliased_variables();
 }

spirv_cross.hpp

@@ -585,6 +585,7 @@ class Compiler
 	// (SSBO, image load store, etc)
 	SmallVector<uint32_t> global_variables;
 	SmallVector<uint32_t> aliased_variables;
+	SmallVector<uint32_t> buffer_pointer_variables;
 
 	SPIRFunction *current_function = nullptr;
 	SPIRBlock *current_block = nullptr;
@@ -705,7 +706,7 @@ class Compiler
 	bool expression_is_lvalue(uint32_t id) const;
 	bool variable_storage_is_aliased(const SPIRVariable &var);
 	SPIRVariable *maybe_get_backing_variable(uint32_t chain);
-	const SPIRExpression *maybe_get_backing_buffer_pointer(uint32_t chain) const;
+	SPIRExpression *maybe_get_backing_buffer_pointer(uint32_t chain);
 
 	void register_read(uint32_t expr, uint32_t chain, bool forwarded);
 	void register_write(uint32_t chain);
@@ -740,6 +741,7 @@ class Compiler
 
 	// Dependency tracking for temporaries read from variables.
 	void flush_dependees(SPIRVariable &var);
+	void flush_dependees(SPIRExpression &expr);
 	void flush_all_active_variables();
 	void flush_control_dependent_expressions(uint32_t block);
 	void flush_all_atomic_capable_variables();

spirv_glsl.cpp

@@ -368,6 +368,7 @@ void CompilerGLSL::reset(uint32_t iteration_count)
 	expression_usage_counts.clear();
 	forwarded_temporaries.clear();
 	suppressed_usage_tracking.clear();
+	buffer_pointer_variables.clear();
 
 	// Ensure that we declare phi-variable copies even if the original declaration isn't deferred
 	flushed_phi_variables.clear();
@@ -8499,20 +8500,25 @@ bool CompilerGLSL::expression_is_constant_null(uint32_t id) const
 	return c->constant_is_null();
 }
 
-bool CompilerGLSL::expression_is_non_value_type_array(uint32_t ptr)
+bool CompilerGLSL::expression_is_non_value_type_array(uint32_t value_type_id, uint32_t ptr)
 {
-	auto &type = expression_type(ptr);
-	if (!is_array(get_pointee_type(type)))
+	auto &type = get<SPIRType>(value_type_id);
+	if (!is_array(type))
 		return false;
 
 	if (!backend.array_is_value_type)
 		return true;
 
+	if (!backend.array_is_value_type_in_buffer_blocks && maybe_get_backing_buffer_pointer(ptr))
+		return true;
+
 	auto *var = maybe_get_backing_variable(ptr);
 	if (!var)
 		return false;
 
 	auto &backed_type = get<SPIRType>(var->basetype);
+
+	// Only consider explicitly laid out types here, not IO blocks.
 	return !backend.array_is_value_type_in_buffer_blocks && backed_type.basetype == SPIRType::Struct &&
 	       has_member_decoration(backed_type.self, 0, DecorationOffset);
 }
@@ -11950,6 +11956,9 @@ bool CompilerGLSL::should_forward(uint32_t id) const
 	if (is_immutable(id))
 		return true;
 
+	if (expr && expr->buffer_pointer)
+		return true;
+
 	return false;
 }
 
@@ -12038,6 +12047,8 @@ void CompilerGLSL::register_impure_function_call()
 		flush_dependees(get<SPIRVariable>(global));
 	for (auto aliased : aliased_variables)
 		flush_dependees(get<SPIRVariable>(aliased));
+	for (auto ptr : buffer_pointer_variables)
+		flush_dependees(get<SPIRExpression>(ptr));
 }
 
 void CompilerGLSL::register_call_out_argument(uint32_t id)
@@ -12935,7 +12946,7 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 		bool usage_tracking = flattened && (type.basetype == SPIRType::Struct || (type.columns > 1));
 
 		SPIRExpression *e = nullptr;
-		if (!forward && expression_is_non_value_type_array(ptr))
+		if (!forward && expression_is_non_value_type_array(result_type, ptr))
 		{
 			// Complicated load case where we need to make a copy of ptr, but we cannot, because
 			// it is an array, and our backend does not support arrays as value types.
@@ -13176,7 +13187,24 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction)
 		expr.buffer_pointer = true;
 		expr.implied_read_expressions = chain_expr->implied_read_expressions;
 		expr.expression_dependencies = chain_expr->expression_dependencies;
+		expr.immutable = false;
+
+		// If the buffer pointer is marked non-writable, ignore alias tracking by flagging the expression as immutable.
+		for (auto &heap : descriptor_heap_types)
+		{
+			if (heap.buffer_pointer_id == result_id)
+			{
+				if (heap.nonwritable)
+					expr.immutable = true;
+				break;
+			}
+		}
+
+		if (!expr.immutable && ir.get_buffer_block_type_flags(get<SPIRType>(type_id)).get(DecorationNonWritable))
+			expr.immutable = true;
 
+		// Used for load-store tracking.
+		buffer_pointer_variables.push_back(result_id);
 		break;
 	}
 

spirv_glsl.hpp

@@ -1059,7 +1059,7 @@ class CompilerGLSL : public Compiler
 	void disallow_forwarding_in_expression_chain(const SPIRExpression &expr);
 
 	bool expression_is_constant_null(uint32_t id) const;
-	bool expression_is_non_value_type_array(uint32_t ptr);
+	bool expression_is_non_value_type_array(uint32_t value_type_id, uint32_t ptr);
 	virtual void emit_store_statement(uint32_t lhs_expression, uint32_t rhs_expression);
 
 	uint32_t get_integer_width_for_instruction(const Instruction &instr) const;