From 3eb55329791efbbc691924cd3533ca459764dd28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Thu, 28 Oct 2021 19:57:41 -0300 Subject: [PATCH 1/7] Add 64 bit support for OpSwitch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit According to the spec, if the `condition` has a type wider than 32 bits, the literals to be compared with will be of that size as well. This caused some misalignments if the `condition` was bigger than 32, causing a nullptr return without further explanation. Currently neither GLSL nor MSL supports uint64 as the condition but the SPIRV allows it anyway. This also fixes #1768. Signed-off-by: Sebastián Aedo --- spirv_common.hpp | 2 +- spirv_parser.cpp | 46 ++++++++++++++++++++++++++++++++++++++++++++-- spirv_parser.hpp | 5 +++++ 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/spirv_common.hpp b/spirv_common.hpp index e602fbd4..1d5e8b20 100644 --- a/spirv_common.hpp +++ b/spirv_common.hpp @@ -854,7 +854,7 @@ struct SPIRBlock : IVariant struct Case { - uint32_t value; + uint64_t value; BlockID block; }; SmallVector cases; diff --git a/spirv_parser.cpp b/spirv_parser.cpp index d50d2e84..51995525 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -771,6 +771,8 @@ void Parser::parse(const Instruction &instruction) uint32_t result_type = ops[0]; uint32_t id = ops[1]; + auto& type = get(ops[0]); + opload_types.insert({ id, type }); // Instead of a temporary, create a new function-wide temporary with this ID instead. auto &var = set(id, result_type, spv::StorageClassFunction); @@ -789,6 +791,7 @@ void Parser::parse(const Instruction &instruction) { uint32_t id = ops[1]; auto &type = get(ops[0]); + opload_types.insert({ id, type }); if (type.width > 32) set(id, ops[0], ops[2] | (uint64_t(ops[3]) << 32), op == OpSpecConstant); @@ -973,10 +976,30 @@ void Parser::parse(const Instruction &instruction) current_block->terminator = SPIRBlock::MultiSelect; current_block->condition = ops[0]; + // If the size of the condition is bigger than 32 bits, all the + // cases should be casted to the size of the condition before + // doing the comparisons. So we need to find out the type of the + // condition first. + auto search = opload_types.find(current_block->condition); + if (search == opload_types.end()) { + SPIRV_CROSS_THROW("The condition has not been declared yet."); + } + + auto &type = search->second; + current_block->default_block = ops[1]; - for (uint32_t i = 2; i + 2 <= length; i += 2) - current_block->cases.push_back({ ops[i], ops[i + 1] }); + if (type.width > 32) { + for (uint32_t i = 2; i + 3 <= length; i += 3) { + uint64_t literal = ((uint64_t)ops[i] << 32) | ops[i+1]; + current_block->cases.push_back({ literal, ops[i+2] }); + } + } else { + for (uint32_t i = 2; i + 2 <= length; i += 2) { + uint64_t literal = ops[i]; + current_block->cases.push_back({ literal, ops[i+1] }); + } + } // If we jump to next block, make it break instead since we're inside a switch case block at that point. ir.block_meta[current_block->next_block] |= ParsedIR::BLOCK_META_MULTISELECT_MERGE_BIT; @@ -1131,6 +1154,25 @@ void Parser::parse(const Instruction &instruction) break; } + case OpConvertFToU: + case OpConvertFToS: + case OpConvertSToF: + case OpConvertUToF: + case OpBitcast: + case OpLoad: + { + // We need to keep track of the OpLoad types for the OpSwitch. + auto &type = get(ops[0]); + opload_types.insert({ ops[1], type }); + + // We still need to trait OpLoad as an actual opcode, so we don't break + if (!current_block) + SPIRV_CROSS_THROW("Currently no block to insert opcode."); + + current_block->ops.push_back(instruction); + break; + } + // Actual opcodes. default: { diff --git a/spirv_parser.hpp b/spirv_parser.hpp index d72fc71d..345588e3 100644 --- a/spirv_parser.hpp +++ b/spirv_parser.hpp @@ -93,6 +93,11 @@ private: SmallVector global_struct_cache; SmallVector> forward_pointer_fixups; + // We need to keep track of all the types that can derive a load because + // the switch can have wider types, but it's not contained in the op + // itself. + std::unordered_map opload_types; + bool types_are_logically_equivalent(const SPIRType &a, const SPIRType &b) const; bool variable_storage_is_aliased(const SPIRVariable &v) const; }; From f099d714f3cfb30660906ed8196a643f06f882bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Tue, 2 Nov 2021 17:17:13 -0300 Subject: [PATCH 2/7] Removing logic in the parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving out the logic from the parser as requested because it's sensitive to try to keep the parsing the most simple process as said. For that, the load_types is now tracked in the ParsedIR, which can be accessed in the Compiler struct. The switch cases are fixed in the CFG stage since that's the point where the nullptr is deref. Signed-off-by: Sebastián Aedo --- spirv_cfg.cpp | 2 +- spirv_common.hpp | 3 +- spirv_cross.cpp | 11 ++++++++ spirv_cross.hpp | 1 + spirv_cross_parsed_ir.cpp | 2 ++ spirv_cross_parsed_ir.hpp | 6 ++++ spirv_parser.cpp | 58 +++++++++++---------------------------- spirv_parser.hpp | 5 ---- 8 files changed, 39 insertions(+), 49 deletions(-) diff --git a/spirv_cfg.cpp b/spirv_cfg.cpp index 9081a664..d239aa19 100644 --- a/spirv_cfg.cpp +++ b/spirv_cfg.cpp @@ -135,6 +135,7 @@ bool CFG::post_order_visit(uint32_t block_id) break; case SPIRBlock::MultiSelect: + compiler.fix_switch_branches(block); for (auto &target : block.cases) { if (post_order_visit(target.block)) @@ -143,7 +144,6 @@ bool CFG::post_order_visit(uint32_t block_id) if (block.default_block && post_order_visit(block.default_block)) add_branch(block_id, block.default_block); break; - default: break; } diff --git a/spirv_common.hpp b/spirv_common.hpp index 1d5e8b20..49ae8069 100644 --- a/spirv_common.hpp +++ b/spirv_common.hpp @@ -857,7 +857,8 @@ struct SPIRBlock : IVariant uint64_t value; BlockID block; }; - SmallVector cases; + mutable SmallVector cases; + SmallVector cases_64bit; // If we have tried to optimize code for this block but failed, // keep track of this. diff --git a/spirv_cross.cpp b/spirv_cross.cpp index 51935061..dadc9fb2 100644 --- a/spirv_cross.cpp +++ b/spirv_cross.cpp @@ -1626,6 +1626,17 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc } } +void Compiler::fix_switch_branches(const SPIRBlock &block) const { + auto search = ir.load_types.find(block.condition); + if (search == ir.load_types.end()) { + SPIRV_CROSS_THROW("Use of undeclared variable on a switch statement."); + } + + const auto& type = search->second; + if (type.width > 32) + block.cases = std::move(block.cases_64bit); +} + bool Compiler::traverse_all_reachable_opcodes(const SPIRBlock &block, OpcodeHandler &handler) const { handler.set_current_block(block); diff --git a/spirv_cross.hpp b/spirv_cross.hpp index 27308601..94af15ff 100644 --- a/spirv_cross.hpp +++ b/spirv_cross.hpp @@ -1117,6 +1117,7 @@ protected: uint32_t evaluate_constant_u32(uint32_t id) const; bool is_vertex_like_shader() const; + void fix_switch_branches(const SPIRBlock &block) const; private: // Used only to implement the old deprecated get_entry_point() interface. diff --git a/spirv_cross_parsed_ir.cpp b/spirv_cross_parsed_ir.cpp index d6cea923..866041bc 100644 --- a/spirv_cross_parsed_ir.cpp +++ b/spirv_cross_parsed_ir.cpp @@ -83,6 +83,7 @@ ParsedIR &ParsedIR::operator=(ParsedIR &&other) SPIRV_CROSS_NOEXCEPT loop_iteration_depth_soft = other.loop_iteration_depth_soft; meta_needing_name_fixup = std::move(other.meta_needing_name_fixup); + load_types = std::move(other.load_types); } return *this; } @@ -116,6 +117,7 @@ ParsedIR &ParsedIR::operator=(const ParsedIR &other) memory_model = other.memory_model; meta_needing_name_fixup = other.meta_needing_name_fixup; + load_types = other.load_types; // Very deliberate copying of IDs. There is no default copy constructor, nor a simple default constructor. // Construct object first so we have the correct allocator set-up, then we can copy object into our new pool group. diff --git a/spirv_cross_parsed_ir.hpp b/spirv_cross_parsed_ir.hpp index 8971a970..168c70f2 100644 --- a/spirv_cross_parsed_ir.hpp +++ b/spirv_cross_parsed_ir.hpp @@ -78,6 +78,12 @@ public: SmallVector ids_for_constant_or_type; SmallVector ids_for_constant_or_variable; + // We need to keep track of all the Ops that contains a type for the + // OpSwitch instruction, since this one doesn't contains the type in the + // instruction itself. And in some case we need to cast the condition to + // wider types. + std::unordered_map load_types; + // Declared capabilities and extensions in the SPIR-V module. // Not really used except for reflection at the moment. SmallVector declared_capabilities; diff --git a/spirv_parser.cpp b/spirv_parser.cpp index 51995525..6e2e3ac8 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -771,8 +771,8 @@ void Parser::parse(const Instruction &instruction) uint32_t result_type = ops[0]; uint32_t id = ops[1]; - auto& type = get(ops[0]); - opload_types.insert({ id, type }); + auto &type = get(result_type); + ir.load_types.insert({ id, type }); // Instead of a temporary, create a new function-wide temporary with this ID instead. auto &var = set(id, result_type, spv::StorageClassFunction); @@ -791,7 +791,7 @@ void Parser::parse(const Instruction &instruction) { uint32_t id = ops[1]; auto &type = get(ops[0]); - opload_types.insert({ id, type }); + ir.load_types.insert({ id, type }); if (type.width > 32) set(id, ops[0], ops[2] | (uint64_t(ops[3]) << 32), op == OpSpecConstant); @@ -976,30 +976,17 @@ void Parser::parse(const Instruction &instruction) current_block->terminator = SPIRBlock::MultiSelect; current_block->condition = ops[0]; - // If the size of the condition is bigger than 32 bits, all the - // cases should be casted to the size of the condition before - // doing the comparisons. So we need to find out the type of the - // condition first. - auto search = opload_types.find(current_block->condition); - if (search == opload_types.end()) { - SPIRV_CROSS_THROW("The condition has not been declared yet."); - } - - auto &type = search->second; - current_block->default_block = ops[1]; - if (type.width > 32) { + uint32_t remaining_ops = length - 2; + for (uint32_t i = 2; i + 2 <= length; i += 2) + current_block->cases.push_back({ ops[i], ops[i + 1] }); + + if (remaining_ops % 3) for (uint32_t i = 2; i + 3 <= length; i += 3) { - uint64_t literal = ((uint64_t)ops[i] << 32) | ops[i+1]; - current_block->cases.push_back({ literal, ops[i+2] }); + uint64_t value = (static_cast(ops[i]) << 32) | ops[i + 1]; + current_block->cases_64bit.push_back({ value, ops[i + 2] }); } - } else { - for (uint32_t i = 2; i + 2 <= length; i += 2) { - uint64_t literal = ops[i]; - current_block->cases.push_back({ literal, ops[i+1] }); - } - } // If we jump to next block, make it break instead since we're inside a switch case block at that point. ir.block_meta[current_block->next_block] |= ParsedIR::BLOCK_META_MULTISELECT_MERGE_BIT; @@ -1154,28 +1141,15 @@ void Parser::parse(const Instruction &instruction) break; } - case OpConvertFToU: - case OpConvertFToS: - case OpConvertSToF: - case OpConvertUToF: - case OpBitcast: - case OpLoad: - { - // We need to keep track of the OpLoad types for the OpSwitch. - auto &type = get(ops[0]); - opload_types.insert({ ops[1], type }); - - // We still need to trait OpLoad as an actual opcode, so we don't break - if (!current_block) - SPIRV_CROSS_THROW("Currently no block to insert opcode."); - - current_block->ops.push_back(instruction); - break; - } - // Actual opcodes. default: { + if (ops) { + const auto *type = maybe_get(ops[0]); + if (type) { + ir.load_types.insert({ ops[1], *type }); + } + } if (!current_block) SPIRV_CROSS_THROW("Currently no block to insert opcode."); diff --git a/spirv_parser.hpp b/spirv_parser.hpp index 345588e3..d72fc71d 100644 --- a/spirv_parser.hpp +++ b/spirv_parser.hpp @@ -93,11 +93,6 @@ private: SmallVector global_struct_cache; SmallVector> forward_pointer_fixups; - // We need to keep track of all the types that can derive a load because - // the switch can have wider types, but it's not contained in the op - // itself. - std::unordered_map opload_types; - bool types_are_logically_equivalent(const SPIRType &a, const SPIRType &b) const; bool variable_storage_is_aliased(const SPIRVariable &v) const; }; From 250a02967dda824d383313c2c88212b94c404849 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Wed, 3 Nov 2021 16:12:14 -0300 Subject: [PATCH 3/7] Removed unnecessary tracking of types. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't need to keep track of the type itself, only its width since the type check of the OpSwitch can be done at runtime. This also avoids creating a dangling reference. Signed-off-by: Sebastián Aedo --- spirv_cross.cpp | 8 ++++---- spirv_cross_parsed_ir.cpp | 5 +++-- spirv_cross_parsed_ir.hpp | 7 ++++--- spirv_parser.cpp | 6 +++--- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/spirv_cross.cpp b/spirv_cross.cpp index dadc9fb2..2c2dacee 100644 --- a/spirv_cross.cpp +++ b/spirv_cross.cpp @@ -1627,13 +1627,13 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc } void Compiler::fix_switch_branches(const SPIRBlock &block) const { - auto search = ir.load_types.find(block.condition); - if (search == ir.load_types.end()) { + auto search = ir.load_type_width.find(block.condition); + if (search == ir.load_type_width.end()) { SPIRV_CROSS_THROW("Use of undeclared variable on a switch statement."); } - const auto& type = search->second; - if (type.width > 32) + const uint32_t width = search->second; + if (width > 32) block.cases = std::move(block.cases_64bit); } diff --git a/spirv_cross_parsed_ir.cpp b/spirv_cross_parsed_ir.cpp index 866041bc..e7fcdff0 100644 --- a/spirv_cross_parsed_ir.cpp +++ b/spirv_cross_parsed_ir.cpp @@ -83,7 +83,7 @@ ParsedIR &ParsedIR::operator=(ParsedIR &&other) SPIRV_CROSS_NOEXCEPT loop_iteration_depth_soft = other.loop_iteration_depth_soft; meta_needing_name_fixup = std::move(other.meta_needing_name_fixup); - load_types = std::move(other.load_types); + load_type_width = std::move(other.load_type_width); } return *this; } @@ -116,8 +116,9 @@ ParsedIR &ParsedIR::operator=(const ParsedIR &other) addressing_model = other.addressing_model; memory_model = other.memory_model; + meta_needing_name_fixup = other.meta_needing_name_fixup; - load_types = other.load_types; + load_type_width = other.load_type_width; // Very deliberate copying of IDs. There is no default copy constructor, nor a simple default constructor. // Construct object first so we have the correct allocator set-up, then we can copy object into our new pool group. diff --git a/spirv_cross_parsed_ir.hpp b/spirv_cross_parsed_ir.hpp index 168c70f2..138d9dd4 100644 --- a/spirv_cross_parsed_ir.hpp +++ b/spirv_cross_parsed_ir.hpp @@ -78,11 +78,12 @@ public: SmallVector ids_for_constant_or_type; SmallVector ids_for_constant_or_variable; - // We need to keep track of all the Ops that contains a type for the + // We need to keep track of the width the Ops that contains a type for the // OpSwitch instruction, since this one doesn't contains the type in the // instruction itself. And in some case we need to cast the condition to - // wider types. - std::unordered_map load_types; + // wider types. We only need the width to do the branch fixup since the + // type check itself can be done at runtime + std::unordered_map load_type_width; // Declared capabilities and extensions in the SPIR-V module. // Not really used except for reflection at the moment. diff --git a/spirv_parser.cpp b/spirv_parser.cpp index 6e2e3ac8..70ecea8f 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -772,7 +772,7 @@ void Parser::parse(const Instruction &instruction) uint32_t result_type = ops[0]; uint32_t id = ops[1]; auto &type = get(result_type); - ir.load_types.insert({ id, type }); + ir.load_type_width.insert({ id, type.width }); // Instead of a temporary, create a new function-wide temporary with this ID instead. auto &var = set(id, result_type, spv::StorageClassFunction); @@ -791,7 +791,7 @@ void Parser::parse(const Instruction &instruction) { uint32_t id = ops[1]; auto &type = get(ops[0]); - ir.load_types.insert({ id, type }); + ir.load_type_width.insert({ id, type.width }); if (type.width > 32) set(id, ops[0], ops[2] | (uint64_t(ops[3]) << 32), op == OpSpecConstant); @@ -1147,7 +1147,7 @@ void Parser::parse(const Instruction &instruction) if (ops) { const auto *type = maybe_get(ops[0]); if (type) { - ir.load_types.insert({ ops[1], *type }); + ir.load_type_width.insert({ ops[1], type->width }); } } if (!current_block) From 48046646eea04a4060862a1f8a0f85b88fd54339 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Mon, 8 Nov 2021 15:18:13 -0300 Subject: [PATCH 4/7] Fixed wrong condition and formatting. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sebastián Aedo --- spirv_cross.cpp | 6 ++++-- spirv_parser.cpp | 11 +++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/spirv_cross.cpp b/spirv_cross.cpp index 2c2dacee..6e18e665 100644 --- a/spirv_cross.cpp +++ b/spirv_cross.cpp @@ -1626,9 +1626,11 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc } } -void Compiler::fix_switch_branches(const SPIRBlock &block) const { +void Compiler::fix_switch_branches(const SPIRBlock &block) const +{ auto search = ir.load_type_width.find(block.condition); - if (search == ir.load_type_width.end()) { + if (search == ir.load_type_width.end()) + { SPIRV_CROSS_THROW("Use of undeclared variable on a switch statement."); } diff --git a/spirv_parser.cpp b/spirv_parser.cpp index 70ecea8f..bdbba471 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -982,8 +982,9 @@ void Parser::parse(const Instruction &instruction) for (uint32_t i = 2; i + 2 <= length; i += 2) current_block->cases.push_back({ ops[i], ops[i + 1] }); - if (remaining_ops % 3) - for (uint32_t i = 2; i + 3 <= length; i += 3) { + if ((remaining_ops % 3) == 0) + for (uint32_t i = 2; i + 3 <= length; i += 3) + { uint64_t value = (static_cast(ops[i]) << 32) | ops[i + 1]; current_block->cases_64bit.push_back({ value, ops[i + 2] }); } @@ -1144,9 +1145,11 @@ void Parser::parse(const Instruction &instruction) // Actual opcodes. default: { - if (ops) { + if (ops) + { const auto *type = maybe_get(ops[0]); - if (type) { + if (type) + { ir.load_type_width.insert({ ops[1], type->width }); } } From 03f678dec4f674090fa8488c980b0d5015cc0552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Thu, 11 Nov 2021 09:55:41 -0300 Subject: [PATCH 5/7] Cast the switch selector in GLSL to uint32_t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can safely cast the value since we check previously that we're not using a uint64_t as the selector. Signed-off-by: Sebastián Aedo --- spirv_glsl.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index 61acda6a..4d74aa30 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -14791,17 +14791,21 @@ void CompilerGLSL::emit_block_chain(SPIRBlock &block) // We only need to consider possible fallthrough if order[i] branches to order[i + 1]. for (auto &c : block.cases) { + // It's safe to cast to uint32_t since we actually do a check + // previously that we're not using uint64_t as the switch selector. + auto case_value = static_cast(c.value); + if (c.block != block.next_block && c.block != block.default_block) { if (!case_constructs.count(c.block)) block_declaration_order.push_back(c.block); - case_constructs[c.block].push_back(c.value); + case_constructs[c.block].push_back(case_value); } else if (c.block == block.next_block && block.default_block != block.next_block) { // We might have to flush phi inside specific case labels. // If we can piggyback on default:, do so instead. - literals_to_merge.push_back(c.value); + literals_to_merge.push_back(case_value); } } From 75e3752273f4e032defa451880f80c2313454d09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Fri, 12 Nov 2021 10:17:38 -0300 Subject: [PATCH 6/7] Added block.cases_32bit and reworked the cases fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now we added block.cases_32bit as requested and we only parse if the remaining ops are a multiple of 2. None of them are mutable because we return a reference of them depending of the op.condition width. Signed-off-by: Sebastián Aedo --- spirv_cfg.cpp | 11 ++++++++--- spirv_common.hpp | 2 +- spirv_cross.cpp | 11 ++++++++--- spirv_cross.hpp | 6 +++++- spirv_glsl.cpp | 5 +++-- spirv_parser.cpp | 11 ++++++++--- 6 files changed, 33 insertions(+), 13 deletions(-) diff --git a/spirv_cfg.cpp b/spirv_cfg.cpp index d239aa19..a938634e 100644 --- a/spirv_cfg.cpp +++ b/spirv_cfg.cpp @@ -135,8 +135,9 @@ bool CFG::post_order_visit(uint32_t block_id) break; case SPIRBlock::MultiSelect: - compiler.fix_switch_branches(block); - for (auto &target : block.cases) + { + const auto &cases = compiler.get_case_list(block); + for (const auto &target : cases) { if (post_order_visit(target.block)) add_branch(block_id, target.block); @@ -144,6 +145,7 @@ bool CFG::post_order_visit(uint32_t block_id) if (block.default_block && post_order_visit(block.default_block)) add_branch(block_id, block.default_block); break; + } default: break; } @@ -385,7 +387,9 @@ void DominatorBuilder::lift_continue_block_dominator() break; case SPIRBlock::MultiSelect: - for (auto &target : block.cases) + { + auto &cases = cfg.get_compiler().get_case_list(block); + for (auto &target : cases) { if (cfg.get_visit_order(target.block) > post_order) back_edge_dominator = true; @@ -393,6 +397,7 @@ void DominatorBuilder::lift_continue_block_dominator() if (block.default_block && cfg.get_visit_order(block.default_block) > post_order) back_edge_dominator = true; break; + } default: break; diff --git a/spirv_common.hpp b/spirv_common.hpp index 49ae8069..bb2260e4 100644 --- a/spirv_common.hpp +++ b/spirv_common.hpp @@ -857,7 +857,7 @@ struct SPIRBlock : IVariant uint64_t value; BlockID block; }; - mutable SmallVector cases; + SmallVector cases_32bit; SmallVector cases_64bit; // If we have tried to optimize code for this block but failed, diff --git a/spirv_cross.cpp b/spirv_cross.cpp index 6e18e665..85e2a755 100644 --- a/spirv_cross.cpp +++ b/spirv_cross.cpp @@ -1626,7 +1626,7 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc } } -void Compiler::fix_switch_branches(const SPIRBlock &block) const +const SmallVector &Compiler::get_case_list(const SPIRBlock &block) const { auto search = ir.load_type_width.find(block.condition); if (search == ir.load_type_width.end()) @@ -1636,7 +1636,9 @@ void Compiler::fix_switch_branches(const SPIRBlock &block) const const uint32_t width = search->second; if (width > 32) - block.cases = std::move(block.cases_64bit); + return block.cases_64bit; + + return block.cases_32bit; } bool Compiler::traverse_all_reachable_opcodes(const SPIRBlock &block, OpcodeHandler &handler) const @@ -3025,12 +3027,15 @@ void Compiler::AnalyzeVariableScopeAccessHandler::set_current_block(const SPIRBl break; case SPIRBlock::MultiSelect: + { notify_variable_access(block.condition, block.self); - for (auto &target : block.cases) + auto &cases = compiler.get_case_list(block); + for (auto &target : cases) test_phi(target.block); if (block.default_block) test_phi(block.default_block); break; + } default: break; diff --git a/spirv_cross.hpp b/spirv_cross.hpp index 94af15ff..3792e2de 100644 --- a/spirv_cross.hpp +++ b/spirv_cross.hpp @@ -1117,7 +1117,11 @@ protected: uint32_t evaluate_constant_u32(uint32_t id) const; bool is_vertex_like_shader() const; - void fix_switch_branches(const SPIRBlock &block) const; + + // Get the correct case list for the OpSwitch, since it can be either a + // 32 bit wide condition or a 64 bit, but the type is not embedded in the + // instruction itself. + const SmallVector &get_case_list(const SPIRBlock &block) const; private: // Used only to implement the old deprecated get_entry_point() interface. diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp index 4d74aa30..a482c403 100644 --- a/spirv_glsl.cpp +++ b/spirv_glsl.cpp @@ -14789,7 +14789,8 @@ void CompilerGLSL::emit_block_chain(SPIRBlock &block) // and let the default: block handle it. // 2.11 in SPIR-V spec states that for fall-through cases, there is a very strict declaration order which we can take advantage of here. // We only need to consider possible fallthrough if order[i] branches to order[i + 1]. - for (auto &c : block.cases) + auto &cases = get_case_list(block); + for (auto &c : cases) { // It's safe to cast to uint32_t since we actually do a check // previously that we're not using uint64_t as the switch selector. @@ -14925,7 +14926,7 @@ void CompilerGLSL::emit_block_chain(SPIRBlock &block) // If there is only one default block, and no cases, this is a case where SPIRV-opt decided to emulate // non-structured exits with the help of a switch block. // This is buggy on FXC, so just emit the logical equivalent of a do { } while(false), which is more idiomatic. - bool degenerate_switch = block.default_block != block.merge_block && block.cases.empty(); + bool degenerate_switch = block.default_block != block.merge_block && block.cases_32bit.empty(); if (degenerate_switch || is_legacy_es()) { diff --git a/spirv_parser.cpp b/spirv_parser.cpp index bdbba471..fc01ce25 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -979,15 +979,20 @@ void Parser::parse(const Instruction &instruction) current_block->default_block = ops[1]; uint32_t remaining_ops = length - 2; - for (uint32_t i = 2; i + 2 <= length; i += 2) - current_block->cases.push_back({ ops[i], ops[i + 1] }); + if ((remaining_ops % 2) == 0) + { + for (uint32_t i = 2; i + 2 <= length; i += 2) + current_block->cases_32bit.push_back({ ops[i], ops[i + 1] }); + } if ((remaining_ops % 3) == 0) + { for (uint32_t i = 2; i + 3 <= length; i += 3) { uint64_t value = (static_cast(ops[i]) << 32) | ops[i + 1]; current_block->cases_64bit.push_back({ value, ops[i + 2] }); } + } // If we jump to next block, make it break instead since we're inside a switch case block at that point. ir.block_meta[current_block->next_block] |= ParsedIR::BLOCK_META_MULTISELECT_MERGE_BIT; @@ -1145,7 +1150,7 @@ void Parser::parse(const Instruction &instruction) // Actual opcodes. default: { - if (ops) + if (length >= 2) { const auto *type = maybe_get(ops[0]); if (type) From 5345051a8551b3fe6bc89bced15ec1a31fc4e60d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Aedo?= Date: Sat, 13 Nov 2021 14:13:30 -0300 Subject: [PATCH 7/7] Removed tracking of OpConstant and OpPhi. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't need to keep track of them because when the block.condition is either a SPIRConstant or a SPIRVariable, we can get the type directly in the get_case_list. Signed-off-by: Sebastián Aedo --- spirv_cross.cpp | 26 ++++++++++++++++++++++---- spirv_parser.cpp | 3 --- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/spirv_cross.cpp b/spirv_cross.cpp index 85e2a755..2b047722 100644 --- a/spirv_cross.cpp +++ b/spirv_cross.cpp @@ -1628,13 +1628,31 @@ SPIRBlock::ContinueBlockType Compiler::continue_block_type(const SPIRBlock &bloc const SmallVector &Compiler::get_case_list(const SPIRBlock &block) const { - auto search = ir.load_type_width.find(block.condition); - if (search == ir.load_type_width.end()) + uint32_t width = 0; + + // First we check if we can get the type directly from the block.condition + // since it can be a SPIRConstant or a SPIRVariable. + if (const auto *constant = maybe_get(block.condition)) { - SPIRV_CROSS_THROW("Use of undeclared variable on a switch statement."); + const auto &type = get(constant->constant_type); + width = type.width; + } + else if (const auto *var = maybe_get(block.condition)) + { + const auto &type = get(var->basetype); + width = type.width; + } + else + { + auto search = ir.load_type_width.find(block.condition); + if (search == ir.load_type_width.end()) + { + SPIRV_CROSS_THROW("Use of undeclared variable on a switch statement."); + } + + width = search->second; } - const uint32_t width = search->second; if (width > 32) return block.cases_64bit; diff --git a/spirv_parser.cpp b/spirv_parser.cpp index fc01ce25..f13e0639 100644 --- a/spirv_parser.cpp +++ b/spirv_parser.cpp @@ -771,8 +771,6 @@ void Parser::parse(const Instruction &instruction) uint32_t result_type = ops[0]; uint32_t id = ops[1]; - auto &type = get(result_type); - ir.load_type_width.insert({ id, type.width }); // Instead of a temporary, create a new function-wide temporary with this ID instead. auto &var = set(id, result_type, spv::StorageClassFunction); @@ -791,7 +789,6 @@ void Parser::parse(const Instruction &instruction) { uint32_t id = ops[1]; auto &type = get(ops[0]); - ir.load_type_width.insert({ id, type.width }); if (type.width > 32) set(id, ops[0], ops[2] | (uint64_t(ops[3]) << 32), op == OpSpecConstant);