From a803e5ae38fd4c32ddd67828d627197a5c8c7b63 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Fri, 9 Mar 2018 15:25:25 +0100 Subject: [PATCH 1/2] Deprecate set_options()/get_options() interface, replace it. Replace with common/hlsl/msl instead. The old interface had some bad interaction with overloading which meant you had to up-cast to base class to be able to use set_options, which was awkward. --- main.cpp | 14 ++++++------ spirv_glsl.hpp | 17 ++++++++++++++ spirv_hlsl.cpp | 60 +++++++++++++++++++++++++------------------------- spirv_hlsl.hpp | 18 ++++++++++++--- spirv_msl.cpp | 18 +++++++-------- spirv_msl.hpp | 20 +++++++++++++---- 6 files changed, 94 insertions(+), 53 deletions(-) diff --git a/main.cpp b/main.cpp index 4f0265d1..4f23c57c 100644 --- a/main.cpp +++ b/main.cpp @@ -748,10 +748,10 @@ static int main_inner(int argc, char *argv[]) compiler = unique_ptr(new CompilerMSL(read_spirv_file(args.input))); auto *msl_comp = static_cast(compiler.get()); - auto msl_opts = msl_comp->get_options(); + auto msl_opts = msl_comp->get_msl_options(); if (args.set_msl_version) msl_opts.msl_version = args.msl_version; - msl_comp->set_options(msl_opts); + msl_comp->set_msl_options(msl_opts); } else if (args.hlsl) compiler = unique_ptr(new CompilerHLSL(read_spirv_file(args.input))); @@ -851,14 +851,14 @@ static int main_inner(int argc, char *argv[]) if (!entry_point.empty()) compiler->set_entry_point(entry_point, model); - if (!args.set_version && !compiler->get_options().version) + if (!args.set_version && !compiler->get_common_options().version) { fprintf(stderr, "Didn't specify GLSL version and SPIR-V did not specify language.\n"); print_help(); return EXIT_FAILURE; } - CompilerGLSL::Options opts = compiler->get_options(); + CompilerGLSL::Options opts = compiler->get_common_options(); if (args.set_version) opts.version = args.version; if (args.set_es) @@ -870,13 +870,13 @@ static int main_inner(int argc, char *argv[]) opts.vulkan_semantics = args.vulkan_semantics; opts.vertex.fixup_clipspace = args.fixup; opts.vertex.flip_vert_y = args.yflip; - compiler->set_options(opts); + compiler->set_common_options(opts); // Set HLSL specific options. if (args.hlsl) { auto *hlsl = static_cast(compiler.get()); - auto hlsl_opts = hlsl->get_options(); + auto hlsl_opts = hlsl->get_hlsl_options(); if (args.set_shader_model) { if (args.shader_model < 30) @@ -894,7 +894,7 @@ static int main_inner(int argc, char *argv[]) hlsl_opts.point_size_compat = true; hlsl_opts.point_coord_compat = true; } - hlsl->set_options(hlsl_opts); + hlsl->set_hlsl_options(hlsl_opts); } if (build_dummy_sampler) diff --git a/spirv_glsl.hpp b/spirv_glsl.hpp index d04654cd..23960021 100644 --- a/spirv_glsl.hpp +++ b/spirv_glsl.hpp @@ -135,15 +135,32 @@ public: init(); } + // Deprecate this interface because it doesn't overload properly with subclasses. + // Requires awkward static casting, which was a mistake. + SPIRV_CROSS_DEPRECATED("get_options() is obsolete, use get_common_options() instead.") const Options &get_options() const { return options; } + + const Options &get_common_options() const + { + return options; + } + + // Deprecate this interface because it doesn't overload properly with subclasses. + // Requires awkward static casting, which was a mistake. + SPIRV_CROSS_DEPRECATED("set_options() is obsolete, use set_common_options() instead.") void set_options(Options &opts) { options = opts; } + void set_common_options(const Options &opts) + { + options = opts; + } + std::string compile() override; // Returns the current string held in the conversion buffer. Useful for diff --git a/spirv_hlsl.cpp b/spirv_hlsl.cpp index c779fabe..a55930c5 100644 --- a/spirv_hlsl.cpp +++ b/spirv_hlsl.cpp @@ -346,7 +346,7 @@ string CompilerHLSL::image_type_hlsl_legacy(const SPIRType &type) string CompilerHLSL::image_type_hlsl(const SPIRType &type) { - if (options.shader_model <= 30) + if (hlsl_options.shader_model <= 30) return image_type_hlsl_legacy(type); else return image_type_hlsl_modern(type); @@ -494,7 +494,7 @@ const char *CompilerHLSL::to_storage_qualifiers_glsl(const SPIRVariable &var) void CompilerHLSL::emit_builtin_outputs_in_struct() { - bool legacy = options.shader_model <= 30; + bool legacy = hlsl_options.shader_model <= 30; for (uint32_t i = 0; i < 64; i++) { if (!(active_output_builtins & (1ull << i))) @@ -551,7 +551,7 @@ void CompilerHLSL::emit_builtin_outputs_in_struct() // If point_size_compat is enabled, just ignore PointSize. // PointSize does not exist in HLSL, but some code bases might want to be able to use these shaders, // even if it means working around the missing feature. - if (options.point_size_compat) + if (hlsl_options.point_size_compat) break; else SPIRV_CROSS_THROW("Unsupported builtin in HLSL."); @@ -568,7 +568,7 @@ void CompilerHLSL::emit_builtin_outputs_in_struct() void CompilerHLSL::emit_builtin_inputs_in_struct() { - bool legacy = options.shader_model <= 30; + bool legacy = hlsl_options.shader_model <= 30; for (uint32_t i = 0; i < 64; i++) { if (!(active_input_builtins & (1ull << i))) @@ -670,7 +670,7 @@ void CompilerHLSL::emit_builtin_inputs_in_struct() case BuiltInPointCoord: // PointCoord is not supported, but provide a way to just ignore that, similar to PointSize. - if (options.point_coord_compat) + if (hlsl_options.point_coord_compat) break; else SPIRV_CROSS_THROW("Unsupported builtin in HLSL."); @@ -789,7 +789,7 @@ void CompilerHLSL::emit_interface_block_in_struct(const SPIRVariable &var, unord string binding; bool use_location_number = true; - bool legacy = options.shader_model <= 30; + bool legacy = hlsl_options.shader_model <= 30; if (execution.model == ExecutionModelFragment && var.storage == StorageClassOutput) { binding = join(legacy ? "COLOR" : "SV_Target", get_decoration(var.self, DecorationLocation)); @@ -909,7 +909,7 @@ void CompilerHLSL::emit_builtin_variables() break; case BuiltInPointSize: - if (options.point_size_compat) + if (hlsl_options.point_size_compat) { // Just emit the global variable, it will be ignored. type = "float"; @@ -1094,7 +1094,7 @@ void CompilerHLSL::emit_resources() } } - if (execution.model == ExecutionModelVertex && options.shader_model <= 30) + if (execution.model == ExecutionModelVertex && hlsl_options.shader_model <= 30) { statement("uniform float4 gl_HalfPixel;"); emitted = true; @@ -1311,7 +1311,7 @@ void CompilerHLSL::emit_resources() if (requires_textureProj) { - if (options.shader_model >= 40) + if (hlsl_options.shader_model >= 40) { statement("float SPIRV_Cross_projectTextureCoordinate(float2 coord)"); begin_scope(); @@ -1819,7 +1819,7 @@ void CompilerHLSL::emit_buffer_block(const SPIRVariable &var) } else { - if (options.shader_model < 51) + if (hlsl_options.shader_model < 51) SPIRV_CROSS_THROW( "Need ConstantBuffer to use arrays of UBOs, but this is only supported in SM 5.1."); @@ -1917,7 +1917,7 @@ string CompilerHLSL::to_func_call_arg(uint32_t id) { string arg_str = CompilerGLSL::to_func_call_arg(id); - if (options.shader_model <= 30) + if (hlsl_options.shader_model <= 30) return arg_str; // Manufacture automatic sampler arg if the arg is a SampledImage texture and we're in modern HLSL. @@ -1997,7 +1997,7 @@ void CompilerHLSL::emit_function_prototype(SPIRFunction &func, uint64_t return_f // Flatten a combined sampler to two separate arguments in modern HLSL. auto &arg_type = get(arg.type); - if (options.shader_model > 30 && arg_type.basetype == SPIRType::SampledImage && arg_type.image.dim != DimBuffer) + if (hlsl_options.shader_model > 30 && arg_type.basetype == SPIRType::SampledImage && arg_type.image.dim != DimBuffer) { // Manufacture automatic sampler arg for SampledImage texture decl += ", "; @@ -2084,7 +2084,7 @@ void CompilerHLSL::emit_hlsl_entry_point() statement(require_output ? "SPIRV_Cross_Output " : "void ", "main(", merge(arguments), ")"); begin_scope(); - bool legacy = options.shader_model <= 30; + bool legacy = hlsl_options.shader_model <= 30; // Copy builtins from entry point arguments to globals. for (uint32_t i = 0; i < 64; i++) @@ -2275,7 +2275,7 @@ void CompilerHLSL::emit_fixup() if (get_entry_point().model == ExecutionModelVertex) { // Do various mangling on the gl_Position. - if (options.shader_model <= 30) + if (hlsl_options.shader_model <= 30) { statement("gl_Position.x = gl_Position.x - gl_HalfPixel.x * " "gl_Position.w;"); @@ -2283,9 +2283,9 @@ void CompilerHLSL::emit_fixup() "gl_Position.w;"); } - if (CompilerGLSL::options.vertex.flip_vert_y) + if (options.vertex.flip_vert_y) statement("gl_Position.y = -gl_Position.y;"); - if (CompilerGLSL::options.vertex.fixup_clipspace) + if (options.vertex.fixup_clipspace) statement("gl_Position.z = (gl_Position.z + gl_Position.w) * 0.5;"); } } @@ -2436,7 +2436,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) if (op == OpImageFetch) { - if (options.shader_model < 40) + if (hlsl_options.shader_model < 40) { SPIRV_CROSS_THROW("texelFetch is not supported in HLSL shader model 2/3."); } @@ -2456,7 +2456,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) SPIRV_CROSS_THROW("Sampling non-float textures is not supported in HLSL."); } - if (options.shader_model >= 40) + if (hlsl_options.shader_model >= 40) { texop += img_expr; @@ -2477,7 +2477,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) else if (gather) { uint32_t comp_num = get(comp).scalar(); - if (options.shader_model >= 50) + if (hlsl_options.shader_model >= 50) { switch (comp_num) { @@ -2555,7 +2555,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) expr += texop; expr += "("; - if (options.shader_model < 40) + if (hlsl_options.shader_model < 40) { if (combined_image) SPIRV_CROSS_THROW("Separate images/samplers are not supported in HLSL shader model 2/3."); @@ -2603,7 +2603,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) coord_expr = "SPIRV_Cross_projectTextureCoordinate(" + coord_expr + ")"; } - if (options.shader_model < 40 && lod) + if (hlsl_options.shader_model < 40 && lod) { auto &coordtype = expression_type(coord); string coord_filler; @@ -2614,7 +2614,7 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) coord_expr = "float4(" + coord_expr + coord_filler + ", " + to_expression(lod) + ")"; } - if (options.shader_model < 40 && bias) + if (hlsl_options.shader_model < 40 && bias) { auto &coordtype = expression_type(coord); string coord_filler; @@ -2652,14 +2652,14 @@ void CompilerHLSL::emit_texture_op(const Instruction &i) expr += to_expression(grad_y); } - if (!dref && lod && options.shader_model >= 40 && op != OpImageFetch) + if (!dref && lod && hlsl_options.shader_model >= 40 && op != OpImageFetch) { forward = forward && should_forward(lod); expr += ", "; expr += to_expression(lod); } - if (!dref && bias && options.shader_model >= 40) + if (!dref && bias && hlsl_options.shader_model >= 40) { forward = forward && should_forward(bias); expr += ", "; @@ -2779,7 +2779,7 @@ string CompilerHLSL::to_resource_binding_sampler(const SPIRVariable &var) string CompilerHLSL::to_resource_register(char space, uint32_t binding, uint32_t space_set) { - if (options.shader_model >= 51) + if (hlsl_options.shader_model >= 51) return join(" : register(", space, binding, ", space", space_set, ")"); else return join(" : register(", space, binding, ")"); @@ -2841,7 +2841,7 @@ void CompilerHLSL::emit_legacy_uniform(const SPIRVariable &var) void CompilerHLSL::emit_uniform(const SPIRVariable &var) { add_resource_name(var.self); - if (options.shader_model >= 40) + if (hlsl_options.shader_model >= 40) emit_modern_uniform(var); else emit_legacy_uniform(var); @@ -3881,7 +3881,7 @@ void CompilerHLSL::emit_instruction(const Instruction &instruction) if (subpass_data) { - if (options.shader_model < 40) + if (hlsl_options.shader_model < 40) SPIRV_CROSS_THROW("Subpass loads are not supported in HLSL shader model 2/3."); // Similar to GLSL, implement subpass loads using texelFetch. @@ -4231,9 +4231,9 @@ uint32_t CompilerHLSL::remap_num_workgroups_builtin() string CompilerHLSL::compile() { // Do not deal with ES-isms like precision, older extensions and such. - CompilerGLSL::options.es = false; - CompilerGLSL::options.version = 450; - CompilerGLSL::options.vulkan_semantics = true; + options.es = false; + options.version = 450; + options.vulkan_semantics = true; backend.float_literal_suffix = true; backend.double_literal_suffix = false; backend.half_literal_suffix = nullptr; diff --git a/spirv_hlsl.hpp b/spirv_hlsl.hpp index a16c3129..fd665f2d 100644 --- a/spirv_hlsl.hpp +++ b/spirv_hlsl.hpp @@ -66,14 +66,26 @@ public: { } + SPIRV_CROSS_DEPRECATED("CompilerHLSL::get_options() is obsolete, use get_hlsl_options() instead.") const Options &get_options() const { - return options; + return hlsl_options; } + const Options &get_hlsl_options() const + { + return hlsl_options; + } + + SPIRV_CROSS_DEPRECATED("CompilerHLSL::get_options() is obsolete, use set_hlsl_options() instead.") void set_options(Options &opts) { - options = opts; + hlsl_options = opts; + } + + void set_hlsl_options(const Options &opts) + { + hlsl_options = opts; } // Optionally specify a custom root constant layout. @@ -151,7 +163,7 @@ private: const char *to_storage_qualifiers_glsl(const SPIRVariable &var) override; - Options options; + Options hlsl_options; bool requires_op_fmod = false; bool requires_textureProj = false; bool requires_fp16_packing = false; diff --git a/spirv_msl.cpp b/spirv_msl.cpp index b4b63713..23601ba4 100644 --- a/spirv_msl.cpp +++ b/spirv_msl.cpp @@ -110,9 +110,9 @@ string CompilerMSL::compile() ClassicLocale classic_locale; // Do not deal with GLES-isms like precision, older extensions and such. - CompilerGLSL::options.vulkan_semantics = true; - CompilerGLSL::options.es = false; - CompilerGLSL::options.version = 450; + options.vulkan_semantics = true; + options.es = false; + options.version = 450; backend.float_literal_suffix = false; backend.half_literal_suffix = "h"; backend.uint32_t_literal_suffix = true; @@ -161,7 +161,7 @@ string CompilerMSL::compile() // Metal does not allow dynamic array lengths. // Resolve any specialization constants that are used for array lengths. - if (options.resolve_specialized_array_lengths) + if (msl_options.resolve_specialized_array_lengths) resolve_specialized_array_lengths(); uint32_t pass_count = 0; @@ -211,7 +211,7 @@ string CompilerMSL::compile(vector *p_vtx_attrs, vector *p_vtx_attrs, vector *p_res_bindings) { - options = msl_cfg; + msl_options = msl_cfg; return compile(p_vtx_attrs, p_res_bindings); } @@ -1844,7 +1844,7 @@ void CompilerMSL::emit_barrier(uint32_t id_exe_scope, uint32_t id_mem_scope, uin else bar_stmt += "mem_none"; - if (options.is_ios() && options.supports_msl_version(2)) + if (msl_options.is_ios() && msl_options.supports_msl_version(2)) { bar_stmt += ", "; @@ -2628,11 +2628,11 @@ void CompilerMSL::emit_fixup() if ((execution.model == ExecutionModelVertex) && stage_out_var_id && !qual_pos_var_name.empty()) { - if (CompilerGLSL::options.vertex.fixup_clipspace) + if (options.vertex.fixup_clipspace) statement(qual_pos_var_name, ".z = (", qual_pos_var_name, ".z + ", qual_pos_var_name, ".w) * 0.5; // Adjust clip-space for Metal"); - if (CompilerGLSL::options.vertex.flip_vert_y) + if (options.vertex.flip_vert_y) statement(qual_pos_var_name, ".y = -(", qual_pos_var_name, ".y);", " // Invert Y-axis for Metal"); } } @@ -2717,7 +2717,7 @@ string CompilerMSL::member_attribute_qualifier(const SPIRType &type, uint32_t in // Some shaders may include a PointSize builtin even when used to render // non-point topologies, and Metal will reject this builtin when compiling // the shader into a render pipeline that uses a non-point topology. - return options.enable_point_size_builtin ? (string(" [[") + builtin_qualifier(builtin) + "]]") : ""; + return msl_options.enable_point_size_builtin ? (string(" [[") + builtin_qualifier(builtin) + "]]") : ""; case BuiltInPosition: case BuiltInLayer: diff --git a/spirv_msl.hpp b/spirv_msl.hpp index 8e6a1130..7765a47d 100644 --- a/spirv_msl.hpp +++ b/spirv_msl.hpp @@ -113,14 +113,26 @@ public: } }; + SPIRV_CROSS_DEPRECATED("CompilerMSL::get_options() is obsolete, use get_msl_options() instead.") const Options &get_options() const { - return options; + return msl_options; } + const Options &get_msl_options() const + { + return msl_options; + } + + SPIRV_CROSS_DEPRECATED("CompilerMSL::set_options() is obsolete, use set_msl_options() instead.") void set_options(Options &opts) { - options = opts; + msl_options = opts; + } + + void set_msl_options(const Options &opts) + { + msl_options = opts; } // An enum of SPIR-V functions that are implemented in additional @@ -174,7 +186,7 @@ public: // This legacy method is deprecated. typedef Options MSLConfiguration; - SPIRV_CROSS_DEPRECATED("Please use get_options() and set_options() instead.") + SPIRV_CROSS_DEPRECATED("Please use get_msl_options() and set_msl_options() instead.") std::string compile(MSLConfiguration &msl_cfg, std::vector *p_vtx_attrs = nullptr, std::vector *p_res_bindings = nullptr); @@ -272,7 +284,7 @@ protected: void build_implicit_builtins(); uint32_t builtin_frag_coord_id = 0; - Options options; + Options msl_options; std::set spv_function_implementations; std::unordered_map vtx_attrs_by_location; std::map non_stage_in_input_var_ids; From 9a52713d77efd517ceb96277dd32042f6110b591 Mon Sep 17 00:00:00 2001 From: Hans-Kristian Arntzen Date: Fri, 9 Mar 2018 15:26:36 +0100 Subject: [PATCH 2/2] Run format_all.sh. --- spirv_glsl.cpp | 2 +- spirv_hlsl.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index c46639bb..d515c37c 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -5820,7 +5820,7 @@ void CompilerGLSL::emit_instruction(const Instruction &instruction) { uint32_t result_type = ops[0]; uint32_t id = ops[1]; - const auto * const elems = &ops[2]; + const auto *const elems = &ops[2]; length -= 2; bool forward = true; diff --git a/spirv_hlsl.cpp b/spirv_hlsl.cpp index a55930c5..b08b9e0e 100644 --- a/spirv_hlsl.cpp +++ b/spirv_hlsl.cpp @@ -1997,7 +1997,8 @@ void CompilerHLSL::emit_function_prototype(SPIRFunction &func, uint64_t return_f // Flatten a combined sampler to two separate arguments in modern HLSL. auto &arg_type = get(arg.type); - if (hlsl_options.shader_model > 30 && arg_type.basetype == SPIRType::SampledImage && arg_type.image.dim != DimBuffer) + if (hlsl_options.shader_model > 30 && arg_type.basetype == SPIRType::SampledImage && + arg_type.image.dim != DimBuffer) { // Manufacture automatic sampler arg for SampledImage texture decl += ", ";