From 6aa2007cba2b98b9c250da9e7be66848e0eed2cd Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Mon, 23 May 2016 12:25:09 +0200 Subject: [PATCH] Deal better with OpName and OpMemberName which alias. OpName is only for debug information, so we must be very careful that we do not reuse the same name for different variables. This was previously done for local variables, but this commit extends this to global variables as well. --- .../shaders/asm/comp/name-alias.asm.comp | 37 ++++++ shaders/asm/comp/name-alias.asm.comp | 122 ++++++++++++++++++ spirv_common.hpp | 3 + spirv_cpp.cpp | 15 ++- spirv_glsl.cpp | 93 +++++++++++-- spirv_glsl.hpp | 14 +- spirv_msl.cpp | 4 +- test_shaders.py | 5 +- 8 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 reference/shaders/asm/comp/name-alias.asm.comp create mode 100644 shaders/asm/comp/name-alias.asm.comp diff --git a/reference/shaders/asm/comp/name-alias.asm.comp b/reference/shaders/asm/comp/name-alias.asm.comp new file mode 100644 index 00000000..4928e7c4 --- /dev/null +++ b/reference/shaders/asm/comp/name-alias.asm.comp @@ -0,0 +1,37 @@ +#version 310 es +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; + +struct alias +{ + vec3 alias[100]; +}; + +struct alias_1 +{ + vec4 alias; + vec2 alias_1[10]; + alias alias_2[2]; +}; + +struct alias_2 +{ + vec4 alias; + alias_1 alias_1; +}; + +layout(binding = 0, std430) buffer _10 +{ + alias_2 alias; +} alias_3; + +layout(binding = 1, std140) buffer _15 +{ + alias_2 alias; +} alias_4; + +void main() +{ + alias_2 alias_5 = alias_3.alias; + alias_4.alias = alias_5; +} + diff --git a/shaders/asm/comp/name-alias.asm.comp b/shaders/asm/comp/name-alias.asm.comp new file mode 100644 index 00000000..5dda2469 --- /dev/null +++ b/shaders/asm/comp/name-alias.asm.comp @@ -0,0 +1,122 @@ +; SPIR-V +; Version: 1.0 +; Generator: Khronos Glslang Reference Front End; 1 +; Bound: 48 +; Schema: 0 + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %4 "main" + OpExecutionMode %4 LocalSize 1 1 1 + OpSource ESSL 310 + OpName %4 "alias" + OpName %15 "alias" + OpMemberName %15 0 "alias" + OpName %18 "alias" + OpMemberName %18 0 "alias" + OpMemberName %18 1 "alias" + OpMemberName %18 2 "alias" + OpName %19 "alias" + OpMemberName %19 0 "alias" + OpMemberName %19 1 "alias" + OpName %21 "alias" + OpName %24 "alias" + OpMemberName %24 0 "alias" + OpName %26 "alias" + OpMemberName %26 0 "alias" + OpMemberName %26 1 "alias" + OpMemberName %26 2 "alias" + OpName %27 "alias" + OpMemberName %27 0 "alias" + OpMemberName %27 1 "alias" + OpName %28 "alias" + OpMemberName %28 0 "alias" + OpName %30 "alias" + OpName %38 "alias" + OpMemberName %38 0 "alias" + OpName %40 "alias" + OpMemberName %40 0 "alias" + OpMemberName %40 1 "alias" + OpMemberName %40 2 "alias" + OpName %41 "alias" + OpMemberName %41 0 "alias" + OpMemberName %41 1 "alias" + OpName %42 "alias" + OpMemberName %42 0 "alias" + OpName %44 "alias" + OpDecorate %22 ArrayStride 8 + OpDecorate %23 ArrayStride 16 + OpMemberDecorate %24 0 Offset 0 + OpDecorate %25 ArrayStride 1600 + OpMemberDecorate %26 0 Offset 0 + OpMemberDecorate %26 1 Offset 16 + OpMemberDecorate %26 2 Offset 96 + OpMemberDecorate %27 0 Offset 0 + OpMemberDecorate %27 1 Offset 16 + OpMemberDecorate %28 0 Offset 0 + OpDecorate %28 BufferBlock + OpDecorate %30 DescriptorSet 0 + OpDecorate %30 Binding 0 + OpDecorate %36 ArrayStride 16 + OpDecorate %37 ArrayStride 16 + OpMemberDecorate %38 0 Offset 0 + OpDecorate %39 ArrayStride 1600 + OpMemberDecorate %40 0 Offset 0 + OpMemberDecorate %40 1 Offset 16 + OpMemberDecorate %40 2 Offset 176 + OpMemberDecorate %41 0 Offset 0 + OpMemberDecorate %41 1 Offset 16 + OpMemberDecorate %42 0 Offset 0 + OpDecorate %42 BufferBlock + OpDecorate %44 DescriptorSet 0 + OpDecorate %44 Binding 1 + %2 = OpTypeVoid + %3 = OpTypeFunction %2 + %6 = OpTypeFloat 32 + %7 = OpTypeVector %6 4 + %8 = OpTypeVector %6 2 + %9 = OpTypeInt 32 0 + %10 = OpConstant %9 10 + %11 = OpTypeArray %8 %10 + %12 = OpTypeVector %6 3 + %13 = OpConstant %9 100 + %14 = OpTypeArray %12 %13 + %15 = OpTypeStruct %14 + %16 = OpConstant %9 2 + %17 = OpTypeArray %15 %16 + %18 = OpTypeStruct %7 %11 %17 + %19 = OpTypeStruct %7 %18 + %20 = OpTypePointer Function %19 + %22 = OpTypeArray %8 %10 + %23 = OpTypeArray %12 %13 + %24 = OpTypeStruct %23 + %25 = OpTypeArray %24 %16 + %26 = OpTypeStruct %7 %22 %25 + %27 = OpTypeStruct %7 %26 + %28 = OpTypeStruct %27 + %29 = OpTypePointer Uniform %28 + %30 = OpVariable %29 Uniform + %31 = OpTypeInt 32 1 + %32 = OpConstant %31 0 + %33 = OpTypePointer Uniform %27 + %36 = OpTypeArray %8 %10 + %37 = OpTypeArray %12 %13 + %38 = OpTypeStruct %37 + %39 = OpTypeArray %38 %16 + %40 = OpTypeStruct %7 %36 %39 + %41 = OpTypeStruct %7 %40 + %42 = OpTypeStruct %41 + %43 = OpTypePointer Uniform %42 + %44 = OpVariable %43 Uniform + %46 = OpTypePointer Uniform %41 + %4 = OpFunction %2 None %3 + %5 = OpLabel + %21 = OpVariable %20 Function + %34 = OpAccessChain %33 %30 %32 + %35 = OpLoad %27 %34 + OpStore %21 %35 + %45 = OpLoad %19 %21 + %47 = OpAccessChain %46 %44 %32 + OpStore %47 %45 + OpReturn + OpFunctionEnd diff --git a/spirv_common.hpp b/spirv_common.hpp index d2d4e378..1ae15c31 100644 --- a/spirv_common.hpp +++ b/spirv_common.hpp @@ -204,6 +204,9 @@ struct SPIRType : IVariant // We want to detect this so that we only emit the struct definition once. // Since we cannot rely on OpName to be equal, we need to figure out aliases. uint32_t type_alias = 0; + + // Used in backends to avoid emitting members with conflicting names. + std::unordered_set member_name_cache; }; struct SPIRExtension : IVariant diff --git a/spirv_cpp.cpp b/spirv_cpp.cpp index 8802846c..b74be5a5 100644 --- a/spirv_cpp.cpp +++ b/spirv_cpp.cpp @@ -22,6 +22,8 @@ using namespace std; void CompilerCPP::emit_buffer_block(const SPIRVariable &var) { + add_resource_name(var.self); + auto &type = get(var.basetype); auto instance_name = to_name(var.self); @@ -38,6 +40,8 @@ void CompilerCPP::emit_buffer_block(const SPIRVariable &var) void CompilerCPP::emit_interface_block(const SPIRVariable &var) { + add_resource_name(var.self); + auto &type = get(var.basetype); const char *qual = var.storage == StorageClassInput ? "StageInput" : "StageOutput"; @@ -57,6 +61,8 @@ void CompilerCPP::emit_interface_block(const SPIRVariable &var) void CompilerCPP::emit_shared(const SPIRVariable &var) { + add_resource_name(var.self); + auto instance_name = to_name(var.self); statement(variable_decl(var), ";"); statement_no_indent("#define ", instance_name, " __res->", instance_name); @@ -64,6 +70,8 @@ void CompilerCPP::emit_shared(const SPIRVariable &var) void CompilerCPP::emit_uniform(const SPIRVariable &var) { + add_resource_name(var.self); + auto &type = get(var.basetype); auto instance_name = to_name(var.self); @@ -93,6 +101,8 @@ void CompilerCPP::emit_uniform(const SPIRVariable &var) void CompilerCPP::emit_push_constant_block(const SPIRVariable &var) { + add_resource_name(var.self); + auto &type = get(var.basetype); auto &flags = meta[var.self].decoration.decoration_flags; if ((flags & (1ull << DecorationBinding)) || (flags & (1ull << DecorationDescriptorSet))) @@ -252,6 +262,7 @@ string CompilerCPP::compile() backend.swizzle_is_function = true; backend.shared_is_implied = true; backend.flexible_member_array_supported = false; + backend.explicit_struct_type = true; uint32_t pass_count = 0; do @@ -355,7 +366,7 @@ string CompilerCPP::constant_expression(const SPIRConstant &c) void CompilerCPP::emit_function_prototype(SPIRFunction &func, uint64_t) { - local_variables.clear(); + local_variable_names = resource_names; string decl; auto &type = get(func.return_type); @@ -374,7 +385,7 @@ void CompilerCPP::emit_function_prototype(SPIRFunction &func, uint64_t) decl += "("; for (auto &arg : func.arguments) { - add_local_variable(arg.id); + add_local_variable_name(arg.id); decl += argument_decl(arg); if (&arg != &func.arguments.back()) diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index 4b269ce6..3241a8e0 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -123,6 +123,8 @@ void CompilerGLSL::reset() expression_usage_counts.clear(); forwarded_temporaries.clear(); + resource_names.clear(); + for (auto &id : ids) { if (id.get_type() == TypeVariable) @@ -350,7 +352,7 @@ void CompilerGLSL::emit_header() statement(""); } -void CompilerGLSL::emit_struct(const SPIRType &type) +void CompilerGLSL::emit_struct(SPIRType &type) { // Struct types can be stamped out multiple times // with just different offsets, matrix layouts, etc ... @@ -359,15 +361,20 @@ void CompilerGLSL::emit_struct(const SPIRType &type) if (type.type_alias != 0) return; + add_resource_name(type.self); auto name = type_to_glsl(type); - statement("struct ", name); + statement(!backend.explicit_struct_type ? "struct " : "", name); begin_scope(); + type.member_name_cache.clear(); + uint32_t i = 0; bool emitted = false; for (auto &member : type.member_types) { + add_member_name(type, i); + auto &membertype = get(member); statement(member_decl(type, membertype, i), ";"); i++; @@ -792,15 +799,28 @@ void CompilerGLSL::emit_buffer_block(const SPIRVariable &var) auto &type = get(var.basetype); auto ssbo = meta[type.self].decoration.decoration_flags & (1ull << DecorationBufferBlock); + add_resource_name(var.self); + // Block names should never alias. auto buffer_name = to_name(type.self, false); + // Shaders never use the block by interface name, so we don't + // have to track this other than updating name caches. + if (resource_names.find(buffer_name) != end(resource_names)) + buffer_name = get_fallback_name(type.self); + else + resource_names.insert(buffer_name); + statement(layout_for_variable(var) + (ssbo ? "buffer " : "uniform ") + buffer_name); begin_scope(); + type.member_name_cache.clear(); + uint32_t i = 0; for (auto &member : type.member_types) { + add_member_name(type, i); + auto &membertype = get(member); statement(member_decl(type, membertype, i), ";"); i++; @@ -827,13 +847,28 @@ void CompilerGLSL::emit_interface_block(const SPIRVariable &var) if (block) { + add_resource_name(var.self); + // Block names should never alias. - statement(layout_for_variable(var), qual, to_name(type.self, false)); + auto block_name = to_name(type.self, false); + + // Shaders never use the block by interface name, so we don't + // have to track this other than updating name caches. + if (resource_names.find(block_name) != end(resource_names)) + block_name = get_fallback_name(type.self); + else + resource_names.insert(block_name); + + statement(layout_for_variable(var), qual, block_name); begin_scope(); + type.member_name_cache.clear(); + uint32_t i = 0; for (auto &member : type.member_types) { + add_member_name(type, i); + auto &membertype = get(member); statement(member_decl(type, membertype, i), ";"); i++; @@ -844,6 +879,7 @@ void CompilerGLSL::emit_interface_block(const SPIRVariable &var) } else { + add_resource_name(var.self); statement(layout_for_variable(var), qual, variable_decl(var), ";"); } } @@ -859,6 +895,7 @@ void CompilerGLSL::emit_uniform(const SPIRVariable &var) throw CompilerError("At least ESSL 3.10 required for shader image load store."); } + add_resource_name(var.self); statement(layout_for_variable(var), "uniform ", variable_decl(var), ";"); } @@ -1053,6 +1090,7 @@ void CompilerGLSL::emit_resources() auto &var = get(global); if (var.storage != StorageClassOutput) { + add_resource_name(var.self); statement(variable_decl(var), ";"); emitted = true; } @@ -3581,6 +3619,26 @@ string CompilerGLSL::to_member_name(const SPIRType &type, uint32_t index) return join("_", index); } +void CompilerGLSL::add_member_name(SPIRType &type, uint32_t index) +{ + auto &memb = meta[type.self].members; + if (index < memb.size() && !memb[index].alias.empty()) + { + auto &name = memb[index].alias; + if (name.empty()) + return; + + // Reserved for temporaries. + if (name[0] == '_' && name.size() >= 2 && isdigit(name[1])) + { + name.clear(); + return; + } + + update_name_cache(type.member_name_cache, name); + } +} + string CompilerGLSL::member_decl(const SPIRType &type, const SPIRType &membertype, uint32_t index) { uint64_t memberflags = 0; @@ -3834,7 +3892,10 @@ string CompilerGLSL::type_to_glsl(const SPIRType &type) { case SPIRType::Struct: // Need OpName lookup here to get a "sensible" name for a struct. - return to_name(type.self); + if (backend.explicit_struct_type) + return join("struct ", to_name(type.self)); + else + return to_name(type.self); case SPIRType::Image: case SPIRType::SampledImage: @@ -3919,20 +3980,30 @@ string CompilerGLSL::type_to_glsl(const SPIRType &type) } } -void CompilerGLSL::add_local_variable(uint32_t id) +void CompilerGLSL::add_variable(unordered_set &variables, uint32_t id) { auto &name = meta[id].decoration.alias; if (name.empty()) return; // Reserved for temporaries. - if (name[0] == '_') + if (name[0] == '_' && name.size() >= 2 && isdigit(name[1])) { name.clear(); return; } - update_name_cache(local_variables, name); + update_name_cache(variables, name); +} + +void CompilerGLSL::add_local_variable_name(uint32_t id) +{ + add_variable(local_variable_names, id); +} + +void CompilerGLSL::add_resource_name(uint32_t id) +{ + add_variable(resource_names, id); } void CompilerGLSL::require_extension(const string &ext) @@ -3971,7 +4042,9 @@ bool CompilerGLSL::check_atomic_image(uint32_t id) void CompilerGLSL::emit_function_prototype(SPIRFunction &func, uint64_t return_flags) { - local_variables.clear(); + // Avoid shadow declarations. + local_variable_names = resource_names; + string decl; auto &type = get(func.return_type); @@ -3994,7 +4067,7 @@ void CompilerGLSL::emit_function_prototype(SPIRFunction &func, uint64_t return_f // SPIRV OpName doesn't have any semantic effect, so it's valid for an implementation // to use same name for variables. // Since we want to make the GLSL debuggable and somewhat sane, use fallback names for variables which are duplicates. - add_local_variable(arg.id); + add_local_variable_name(arg.id); decl += argument_decl(arg); if (&arg != &func.arguments.back()) @@ -4045,7 +4118,7 @@ void CompilerGLSL::emit_function(SPIRFunction &func, uint64_t return_flags) auto &var = get(v); if (expression_is_lvalue(v)) { - add_local_variable(var.self); + add_local_variable_name(var.self); if (var.initializer) statement(variable_decl(var), ";"); diff --git a/spirv_glsl.hpp b/spirv_glsl.hpp index bdde7aaa..0b3c9a77 100644 --- a/spirv_glsl.hpp +++ b/spirv_glsl.hpp @@ -188,8 +188,12 @@ protected: std::string type_to_array_glsl(const SPIRType &type); std::string variable_decl(const SPIRVariable &variable); - void add_local_variable(uint32_t id); - std::unordered_set local_variables; + void add_local_variable_name(uint32_t id); + void add_resource_name(uint32_t id); + void add_member_name(SPIRType &type, uint32_t name); + std::unordered_set local_variable_names; + std::unordered_set resource_names; + std::unordered_set interface_block_names; bool processing_entry_point = false; @@ -204,12 +208,12 @@ protected: bool swizzle_is_function = false; bool shared_is_implied = false; bool flexible_member_array_supported = true; + bool explicit_struct_type = false; } backend; - void emit_struct(const SPIRType &type); + void emit_struct(SPIRType &type); void emit_instruction(const Instruction &instr); -protected: void emit_resources(); void emit_buffer_block(const SPIRVariable &type); void emit_push_constant_block(const SPIRVariable &var); @@ -320,6 +324,8 @@ protected: const char *to_pls_qualifiers_glsl(const SPIRVariable &variable); void emit_pls(); void remap_pls_variables(); + + void add_variable(std::unordered_set &variables, uint32_t id); }; } diff --git a/spirv_msl.cpp b/spirv_msl.cpp index 226a8193..51eabf01 100644 --- a/spirv_msl.cpp +++ b/spirv_msl.cpp @@ -471,7 +471,7 @@ void CompilerMSL::emit_function_prototype(SPIRFunction &func, uint64_t) // If this is the entry point function, Metal-specific return value and function arguments are added. void CompilerMSL::emit_function_prototype(SPIRFunction &func, bool is_decl) { - local_variables.clear(); + local_variable_names = resource_names; string decl; processing_entry_point = (func.self == execution.entry_point); @@ -500,7 +500,7 @@ void CompilerMSL::emit_function_prototype(SPIRFunction &func, bool is_decl) for (auto &arg : func.arguments) { - add_local_variable(arg.id); + add_local_variable_name(arg.id); decl += "thread " + argument_decl(arg); if (&arg != &func.arguments.back()) diff --git a/test_shaders.py b/test_shaders.py index 68785eee..8b523caa 100755 --- a/test_shaders.py +++ b/test_shaders.py @@ -81,8 +81,9 @@ def cross_compile(shader, vulkan, spirv): else: subprocess.check_call(['glslangValidator', '-V' if vulkan else '-G', '-o', spirv_path, shader]) - if spirv: - subprocess.check_call(['spirv-val', spirv_path]) + # Workaround Issue #217 in SPIRV-Tools until the issue is resolved. + #if spirv: + # subprocess.check_call(['spirv-val', spirv_path]) spirv_cross_path = './spirv-cross' subprocess.check_call([spirv_cross_path, '--output', glsl_path, spirv_path])