From 4a421912401758d390e551088324552ab61d822e Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 2 Nov 2023 23:20:22 -0400 Subject: [PATCH] MSL: Fix regression error in argument buffer runtime arrays. Argument buffers can contain multiple runtime arrays if they have fixed lengths as specified by the binding API. Regression error had assumed each runtime array is in separate argument buffer with undefined array length. - Add CompilerMSL::is_var_runtime_size_array() to include test for setting of array length via CompilerMSL::add_msl_resource_binding(). - Fixed unrelated test case MSL compile syntax failure when acceleration structure is the first entry point function argument (unrelated). --- .../ray-query-mutability.spv14.vk.msl24.frag | 2 +- spirv_msl.cpp | 38 +++++++++++-------- spirv_msl.hpp | 1 + 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/reference/shaders-msl-no-opt/frag/ray-query-mutability.spv14.vk.msl24.frag b/reference/shaders-msl-no-opt/frag/ray-query-mutability.spv14.vk.msl24.frag index 720fba1e5..09184a457 100644 --- a/reference/shaders-msl-no-opt/frag/ray-query-mutability.spv14.vk.msl24.frag +++ b/reference/shaders-msl-no-opt/frag/ray-query-mutability.spv14.vk.msl24.frag @@ -61,7 +61,7 @@ uint proceeFn(thread raytracing::intersection_query topLevelAS [[buffer(0)]]) +fragment void main0(raytracing::acceleration_structure topLevelAS [[buffer(0)]]) { raytracing::intersection_query rayQuery; initFn(rayQuery, topLevelAS); diff --git a/spirv_msl.cpp b/spirv_msl.cpp index b52750e85..db2f77b18 100644 --- a/spirv_msl.cpp +++ b/spirv_msl.cpp @@ -189,6 +189,11 @@ bool CompilerMSL::is_msl_resource_binding_used(ExecutionModel model, uint32_t de return itr != end(resource_bindings) && itr->second.second; } +bool CompilerMSL::is_var_runtime_size_array(const SPIRVariable &var) const +{ + return is_runtime_size_array(get_variable_data_type(var)) && get_resource_array_size(var.self) == 0; +} + // Returns the size of the array of resources used by the variable with the specified id. // The returned value is retrieved from the resource binding added using add_msl_resource_binding(). uint32_t CompilerMSL::get_resource_array_size(uint32_t id) const @@ -1361,7 +1366,7 @@ void CompilerMSL::emit_entry_point_declarations() const auto &type = get_variable_data_type(var); const auto &buffer_type = get_variable_element_type(var); const string name = to_name(var.self); - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) { if (msl_options.argument_buffers_tier < Options::ArgumentBuffersTier::Tier2) { @@ -10503,7 +10508,7 @@ void CompilerMSL::emit_function_prototype(SPIRFunction &func, const Bitset &) // Manufacture automatic sampler arg for SampledImage texture if (arg_type.image.dim != DimBuffer) { - if (arg_type.array.empty() || is_runtime_size_array(arg_type)) + if (arg_type.array.empty() || (var ? is_var_runtime_size_array(*var) : is_runtime_size_array(arg_type))) { decl += join(", ", sampler_type(arg_type, arg.id), " ", to_sampler_expression(name_id)); } @@ -11693,8 +11698,7 @@ string CompilerMSL::to_buffer_size_expression(uint32_t id) auto array_expr = expr.substr(index); if (auto var = maybe_get_backing_variable(id)) { - auto &var_type = get(var->basetype); - if (is_runtime_size_array(var_type)) + if (is_var_runtime_size_array(*var)) { if (!msl_options.runtime_array_rich_descriptor) SPIRV_CROSS_THROW("OpArrayLength requires rich descriptor format"); @@ -13227,7 +13231,7 @@ void CompilerMSL::entry_point_args_discrete_descriptors(string &ep_args) uint32_t array_size = to_array_size_literal(type); is_using_builtin_array = true; - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) { add_spv_func_and_recompile(SPVFuncImplVariableDescriptorArray); if (!ep_args.empty()) @@ -13289,7 +13293,7 @@ void CompilerMSL::entry_point_args_discrete_descriptors(string &ep_args) if (!ep_args.empty()) ep_args += ", "; ep_args += sampler_type(type, var_id) + " " + r.name; - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) ep_args += "_ [[buffer(" + convert_to_string(r.index) + ")]]"; else ep_args += " [[sampler(" + convert_to_string(r.index) + ")]]"; @@ -13307,7 +13311,7 @@ void CompilerMSL::entry_point_args_discrete_descriptors(string &ep_args) if (r.plane > 0) ep_args += join(plane_name_suffix, r.plane); - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) ep_args += "_ [[buffer(" + convert_to_string(r.index) + ")"; else ep_args += " [[texture(" + convert_to_string(r.index) + ")"; @@ -13338,17 +13342,21 @@ void CompilerMSL::entry_point_args_discrete_descriptors(string &ep_args) } case SPIRType::AccelerationStructure: { - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) { add_spv_func_and_recompile(SPVFuncImplVariableDescriptor); const auto &parent_type = get(type.parent_type); - ep_args += ", const device spvDescriptor<" + type_to_glsl(parent_type) + ">* " + + if (!ep_args.empty()) + ep_args += ", "; + ep_args += "const device spvDescriptor<" + type_to_glsl(parent_type) + ">* " + to_restrict(var_id, true) + r.name + "_"; ep_args += " [[buffer(" + convert_to_string(r.index) + ")]]"; } else { - ep_args += ", " + type_to_glsl(type, var_id) + " " + r.name; + if (!ep_args.empty()) + ep_args += ", "; + ep_args += type_to_glsl(type, var_id) + " " + r.name; ep_args += " [[buffer(" + convert_to_string(r.index) + ")]]"; } break; @@ -13440,7 +13448,7 @@ void CompilerMSL::fix_up_shader_inputs_outputs() entry_func.fixup_hooks_in.push_back( [this, &type, &var, var_id]() { - bool is_array_type = !type.array.empty() && !is_runtime_size_array(type); + bool is_array_type = !type.array.empty() && !is_var_runtime_size_array(var); uint32_t desc_set = get_decoration(var_id, DecorationDescriptorSet); if (descriptor_set_is_argument_buffer(desc_set)) @@ -14056,7 +14064,7 @@ uint32_t CompilerMSL::get_metal_resource_index(SPIRVariable &var, SPIRType::Base } else { - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) { basetype = SPIRType::Struct; binding_stride = 1; @@ -14214,7 +14222,7 @@ string CompilerMSL::argument_decl(const SPIRFunction::Parameter &arg) else decl = join(cv_qualifier, type_to_glsl(type, arg.id)); } - else if (is_runtime_size_array(type)) + else if (is_var_runtime_size_array(var)) { const auto *parent_type = &get(type.parent_type); auto type_name = type_to_glsl(*parent_type, arg.id); @@ -14316,7 +14324,7 @@ string CompilerMSL::argument_decl(const SPIRFunction::Parameter &arg) decl += join("[", array_size, "]"); } } - else if (is_runtime_size_array(type)) + else if (is_var_runtime_size_array(var)) { decl += " " + to_expression(name_id); } @@ -14366,7 +14374,7 @@ string CompilerMSL::argument_decl(const SPIRFunction::Parameter &arg) } else if (type_is_image || type_is_tlas) { - if (is_runtime_size_array(type)) + if (is_var_runtime_size_array(var)) { decl = address_space + " " + decl + " " + to_expression(name_id); } diff --git a/spirv_msl.hpp b/spirv_msl.hpp index dc1495301..b9d360697 100644 --- a/spirv_msl.hpp +++ b/spirv_msl.hpp @@ -970,6 +970,7 @@ class CompilerMSL : public CompilerGLSL void emit_specialization_constants_and_structs(); void emit_interface_block(uint32_t ib_var_id); bool maybe_emit_array_assignment(uint32_t id_lhs, uint32_t id_rhs); + bool is_var_runtime_size_array(const SPIRVariable &var) const; uint32_t get_resource_array_size(uint32_t id) const; void fix_up_shader_inputs_outputs();