diff --git a/source/val/BasicBlock.cpp b/source/val/BasicBlock.cpp index 432573622..7e66803a0 100644 --- a/source/val/BasicBlock.cpp +++ b/source/val/BasicBlock.cpp @@ -26,6 +26,7 @@ #include "BasicBlock.h" +#include #include using std::vector; @@ -75,6 +76,14 @@ void BasicBlock::RegisterBranchInstruction(SpvOp branch_instruction) { return; } +void BasicBlock::SetSuccessorsUnsafe(std::vector&& others) { + successors_ = std::move(others); +} + +void BasicBlock::SetPredecessorsUnsafe(std::vector&& others) { + predecessors_ = std::move(others); +} + BasicBlock::DominatorIterator::DominatorIterator() : current_(nullptr) {} BasicBlock::DominatorIterator::DominatorIterator( diff --git a/source/val/BasicBlock.h b/source/val/BasicBlock.h index 8818faa54..d421ae8c5 100644 --- a/source/val/BasicBlock.h +++ b/source/val/BasicBlock.h @@ -123,6 +123,14 @@ class BasicBlock { /// Adds @p next BasicBlocks as successors of this BasicBlock void RegisterSuccessors(const std::vector& next = {}); + /// Set the successors to this block, without updating other internal state, + /// and without updating the other blocks. + void SetSuccessorsUnsafe(std::vector&& others); + + /// Set the predecessors to this block, without updating other internal state, + /// and without updating the other blocks. + void SetPredecessorsUnsafe(std::vector&& others); + /// Returns true if the id of the BasicBlock matches bool operator==(const BasicBlock& other) const { return other.id_ == id_; } diff --git a/source/val/Function.cpp b/source/val/Function.cpp index d2c89fbf5..ebb1e9f12 100644 --- a/source/val/Function.cpp +++ b/source/val/Function.cpp @@ -69,9 +69,13 @@ Function::Function(uint32_t id, uint32_t result_type_id, result_type_id_(result_type_id), function_control_(function_control), declaration_type_(FunctionDecl::kFunctionDeclUnknown), + end_has_been_registered_(false), blocks_(), current_block_(nullptr), + pseudo_entry_block_(0), pseudo_exit_block_(kInvalidId), + pseudo_entry_blocks_({&pseudo_entry_block_}), + pseudo_exit_blocks_({&pseudo_exit_block_}), cfg_constructs_(), variable_ids_(), parameter_ids_() {} @@ -206,17 +210,29 @@ void Function::RegisterBlockEnd(vector next_list, next_blocks.push_back(&inserted_block->second); } - if (branch_instruction == SpvOpReturn || - branch_instruction == SpvOpReturnValue) { - assert(next_blocks.empty()); - next_blocks.push_back(&pseudo_exit_block_); - } current_block_->RegisterBranchInstruction(branch_instruction); current_block_->RegisterSuccessors(next_blocks); current_block_ = nullptr; return; } +void Function::RegisterFunctionEnd() { + if (!end_has_been_registered_) { + end_has_been_registered_ = true; + + // Compute the successors of the pseudo-entry block, and + // the predecessors of the pseudo exit block. + vector sources; + vector sinks; + for (const auto b : ordered_blocks_) { + if (b->get_predecessors()->empty()) sources.push_back(b); + if (b->get_successors()->empty()) sinks.push_back(b); + } + pseudo_entry_block_.SetSuccessorsUnsafe(std::move(sources)); + pseudo_exit_block_.SetPredecessorsUnsafe(std::move(sinks)); + } +} + size_t Function::get_block_count() const { return blocks_.size(); } size_t Function::get_undefined_block_count() const { @@ -231,6 +247,11 @@ vector& Function::get_blocks() { return ordered_blocks_; } const BasicBlock* Function::get_current_block() const { return current_block_; } BasicBlock* Function::get_current_block() { return current_block_; } +BasicBlock* Function::get_pseudo_entry_block() { return &pseudo_entry_block_; } +const BasicBlock* Function::get_pseudo_entry_block() const { + return &pseudo_entry_block_; +} + BasicBlock* Function::get_pseudo_exit_block() { return &pseudo_exit_block_; } const BasicBlock* Function::get_pseudo_exit_block() const { return &pseudo_exit_block_; diff --git a/source/val/Function.h b/source/val/Function.h index 4344fe9d6..5552cfe76 100644 --- a/source/val/Function.h +++ b/source/val/Function.h @@ -95,11 +95,14 @@ class Function { /// Registers the end of the block /// - /// @param[in] successors_list A list of ids to the blocks successors + /// @param[in] successors_list A list of ids to the block's successors /// @param[in] branch_instruction the branch instruction that ended the block void RegisterBlockEnd(std::vector successors_list, SpvOp branch_instruction); + /// Registers the end of the function. This is idempotent. + void RegisterFunctionEnd(); + /// Returns true if the \p id block is the first block of this function bool IsFirstBlock(uint32_t id) const; @@ -149,12 +152,32 @@ class Function { /// Returns the block that is currently being parsed in the binary const BasicBlock* get_current_block() const; - /// Returns the psudo exit block + /// Returns the pseudo exit block + BasicBlock* get_pseudo_entry_block(); + + /// Returns the pseudo exit block + const BasicBlock* get_pseudo_entry_block() const; + + /// Returns the pseudo exit block BasicBlock* get_pseudo_exit_block(); - /// Returns the psudo exit block + /// Returns the pseudo exit block const BasicBlock* get_pseudo_exit_block() const; + /// Returns a vector with just the pseudo entry block. + /// This serves as the predecessors of each source node in the CFG when computing + /// dominators. + const std::vector* get_pseudo_entry_blocks() const { + return &pseudo_entry_blocks_; + } + + /// Returns a vector with just the pseudo exit block. + /// This serves as the successors of each sink node in the CFG when computing + /// dominators. + const std::vector* get_pseudo_exit_blocks() const { + return &pseudo_exit_blocks_; + } + /// Prints a GraphViz digraph of the CFG of the current funciton void printDotGraph() const; @@ -180,6 +203,9 @@ class Function { /// The type of declaration of each function FunctionDecl declaration_type_; + // Have we finished parsing this function? + bool end_has_been_registered_; + /// The blocks in the function mapped by block ID std::unordered_map blocks_; @@ -192,9 +218,29 @@ class Function { /// The block that is currently being parsed BasicBlock* current_block_; - /// A pseudo exit block that is the successor to all return blocks + /// A pseudo entry block that, for the purposes of dominance analysis, + /// is considered the predecessor to any ordinary block without predecessors. + /// After the function end has been registered, its successor list consists + /// of all ordinary blocks without predecessors. It has no predecessors. + /// It does not appear in the predecessor or successor list of any + /// ordinary block. + /// It has Id 0. + BasicBlock pseudo_entry_block_; + + /// A pseudo exit block that, for the purposes of dominance analysis, + /// is considered the successor to any ordinary block without successors. + /// After the function end has been registered, its predecessor list consists + /// of all ordinary blocks without successors. It has no successors. + /// It does not appear in the predecessor or successor list of any + /// ordinary block. BasicBlock pseudo_exit_block_; + // A vector containing pseudo_entry_block_. + const std::vector pseudo_entry_blocks_; + + // A vector containing pseudo_exit_block_. + const std::vector pseudo_exit_blocks_; + /// The constructs that are available in this function std::list cfg_constructs_; diff --git a/source/val/ValidationState.cpp b/source/val/ValidationState.cpp index cf7193c85..d09dbaa58 100644 --- a/source/val/ValidationState.cpp +++ b/source/val/ValidationState.cpp @@ -353,6 +353,7 @@ spv_result_t ValidationState_t::RegisterFunctionEnd() { assert(in_block() == false && "RegisterFunctionParameter can only be called when parsing the binary " "ouside of a block"); + get_current_function().RegisterFunctionEnd(); in_function_ = false; return SPV_SUCCESS; } diff --git a/source/validate.cpp b/source/validate.cpp index 63f7fe040..6e153be42 100644 --- a/source/validate.cpp +++ b/source/validate.cpp @@ -185,6 +185,10 @@ spv_result_t spvValidate(const spv_const_context context, binary->wordCount, setHeader, ProcessInstruction, pDiagnostic)); + if (vstate.in_function_body()) + return vstate.diag(SPV_ERROR_INVALID_LAYOUT) + << "Missing OpFunctionEnd at end of module."; + // TODO(umar): Add validation checks which require the parsing of the entire // module. Use the information from the ProcessInstruction pass to make the // checks. diff --git a/source/validate.h b/source/validate.h index 74b350688..e24f7b11e 100644 --- a/source/validate.h +++ b/source/validate.h @@ -60,6 +60,12 @@ using get_blocks_func = /// @brief Calculates dominator edges for a set of blocks /// +/// Computes dominators using the algorithm of Cooper, Harvey, and Kennedy +/// "A Simple, Fast Dominance Algorithm", 2001. +/// +/// The algorithm assumes there is a unique root node (a node without predecessors), +/// and it is therefore at the end of the postorder vector. +/// /// This function calculates the dominator edges for a set of blocks in the CFG. /// Uses the dominator algorithm by Cooper et al. /// @@ -68,7 +74,10 @@ using get_blocks_func = /// @param[in] predecessor_func Function used to get the predecessor nodes of a /// block /// -/// @return a set of dominator edges represented as a pair of blocks +/// @return the dominator tree of the graph, as a vector of pairs of nodes. +/// The first node in the pair is a node in the graph. The second node in the pair +/// is its immediate dominator in the sense of Cooper et.al., where a block without +/// predecessors (such as the root node) is its own immediate dominator. std::vector> CalculateDominators( const std::vector& postorder, get_blocks_func predecessor_func); diff --git a/source/validate_cfg.cpp b/source/validate_cfg.cpp index b687661d8..2402ec1ff 100644 --- a/source/validate_cfg.cpp +++ b/source/validate_cfg.cpp @@ -86,14 +86,15 @@ bool FindInWorkList(const vector& work_list, uint32_t id) { return false; } + /// @brief Depth first traversal starting from the \p entry BasicBlock /// /// This function performs a depth first traversal from the \p entry /// BasicBlock and calls the pre/postorder functions when it needs to process /// the node in pre order, post order. It also calls the backedge function -/// when a back edge is encountered +/// when a back edge is encountered. /// -/// @param[in] entry The root BasicBlock of a CFG tree +/// @param[in] entry The root BasicBlock of a CFG /// @param[in] successor_func A function which will return a pointer to the /// successor nodes /// @param[in] preorder A function that will be called for every block in a @@ -102,22 +103,24 @@ bool FindInWorkList(const vector& work_list, uint32_t id) { /// CFG following postorder traversal semantics /// @param[in] backedge A function that will be called when a backedge is /// encountered during a traversal -/// NOTE: The @p successor_func return a pointer to a collection such that -/// iterators to that collection remain valid for the lifetime of the algorithm -void DepthFirstTraversal(const BasicBlock& entry, +/// NOTE: The @p successor_func and predecessor_func each return a pointer to a +/// collection such that iterators to that collection remain valid for the lifetime +/// of the algorithm +void DepthFirstTraversal(const BasicBlock* entry, get_blocks_func successor_func, function preorder, function postorder, function backedge) { - vector out; unordered_set processed; - /// NOTE: work_list is the sequence of nodes from the entry node to the node + + /// NOTE: work_list is the sequence of nodes from the root node to the node /// being processed in the traversal vector work_list; - work_list.reserve(10); - work_list.push_back({&entry, begin(*successor_func(&entry))}); - preorder(&entry); + + work_list.push_back({entry, begin(*successor_func(entry))}); + preorder(entry); + processed.insert(entry->get_id()); while (!work_list.empty()) { block_info& top = work_list.back(); @@ -140,19 +143,9 @@ void DepthFirstTraversal(const BasicBlock& entry, } } -/// Returns the successor of a basic block. -/// NOTE: This will be passed as a function pointer to when calculating -/// the dominator and post dominator -const vector* successor(const BasicBlock* b) { - return b->get_successors(); -} - -const vector* predecessor(const BasicBlock* b) { - return b->get_predecessors(); -} - } // namespace + vector> CalculateDominators( const vector& postorder, get_blocks_func predecessor_func) { struct block_detail { @@ -170,21 +163,20 @@ vector> CalculateDominators( bool changed = true; while (changed) { changed = false; - for (auto b = postorder.rbegin() + 1; b != postorder.rend(); b++) { - const vector* predecessors = predecessor_func(*b); + for (auto b = postorder.rbegin() + 1; b != postorder.rend(); ++b) { + const vector& predecessors = *predecessor_func(*b); // first processed/reachable predecessor - auto res = find_if(begin(*predecessors), end(*predecessors), + auto res = find_if(begin(predecessors), end(predecessors), [&idoms, undefined_dom](BasicBlock* pred) { - return idoms[pred].dominator != undefined_dom && - pred->is_reachable(); + return idoms[pred].dominator != undefined_dom; }); - if (res == end(*predecessors)) continue; - BasicBlock* idom = *res; + if (res == end(predecessors)) continue; + const BasicBlock* idom = *res; size_t idom_idx = idoms[idom].postorder_index; // all other predecessors - for (auto p : *predecessors) { - if (idom == p || p->is_reachable() == false) continue; + for (const auto* p : predecessors) { + if (idom == p) continue; if (idoms[p].dominator != undefined_dom) { size_t finger1 = idoms[p].postorder_index; size_t finger2 = idom_idx; @@ -216,14 +208,6 @@ vector> CalculateDominators( return out; } -void UpdateImmediateDominators( - const vector>& dom_edges, - function set_func) { - for (auto& edge : dom_edges) { - set_func(get<0>(edge), get<1>(edge)); - } -} - void printDominatorList(const BasicBlock& b) { std::cout << b.get_id() << " is dominated by: "; const BasicBlock* bb = &b; @@ -408,35 +392,71 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) { << _.getIdName(function.get_id()); } - // Updates each blocks immediate dominators + // Prepare for dominance calculations. We want to analyze all the + // blocks in the function, even in degenerate control flow cases + // including unreachable blocks. For this calculation, we create an + // agumented CFG by adding a pseudo-entry node that is considered the + // predecessor of each source in the original CFG, and a pseudo-exit + // node that is considered the successor to each sink in the original + // CFG. The augmented CFG is guaranteed to have a single source node + // (i.e. not having predecessors) and a single exit node (i.e. not + // having successors). However, there might be isolated strongly + // connected components that are not reachable by following successors + // from the pseudo entry node, and not reachable by following + // predecessors from the pseudo exit node. + + auto* pseudo_entry = function.get_pseudo_entry_block(); + auto* pseudo_exit = function.get_pseudo_exit_block(); + // We need vectors to use as the predecessors (in the augmented CFG) + // for the source nodes of the original CFG. It must have a stable + // address for the duration of the calculation. + auto* pseudo_entry_vec = function.get_pseudo_entry_blocks(); + // Similarly, we need a vector to be used as the successors (in the + // augmented CFG) for sinks in the original CFG. + auto* pseudo_exit_vec = function.get_pseudo_exit_blocks(); + // Returns the predecessors of a block in the augmented CFG. + auto augmented_predecessor_fn = [pseudo_entry, pseudo_entry_vec]( + const BasicBlock* block) { + auto predecessors = block->get_predecessors(); + return (block != pseudo_entry && predecessors->empty()) ? pseudo_entry_vec + : predecessors; + }; + // Returns the successors of a block in the augmented CFG. + auto augmented_successor_fn = [pseudo_exit, + pseudo_exit_vec](const BasicBlock* block) { + auto successors = block->get_successors(); + return (block != pseudo_exit && successors->empty()) ? pseudo_exit_vec + : successors; + }; + + // Set each block's immediate dominator and immediate postdominator, + // and find all back-edges. vector postorder; vector postdom_postorder; vector> back_edges; - if (auto* first_block = function.get_first_block()) { + if (!function.get_blocks().empty()) { /// calculate dominators - DepthFirstTraversal(*first_block, successor, [](cbb_ptr) {}, + DepthFirstTraversal(pseudo_entry, augmented_successor_fn, [](cbb_ptr) {}, [&](cbb_ptr b) { postorder.push_back(b); }, [&](cbb_ptr from, cbb_ptr to) { back_edges.emplace_back(from->get_id(), to->get_id()); }); - auto edges = libspirv::CalculateDominators(postorder, predecessor); - libspirv::UpdateImmediateDominators( - edges, [](bb_ptr block, bb_ptr dominator) { - block->SetImmediateDominator(dominator); - }); + auto edges = + libspirv::CalculateDominators(postorder, augmented_predecessor_fn); + for (auto edge : edges) { + edge.first->SetImmediateDominator(edge.second); + } /// calculate post dominators - auto exit_block = function.get_pseudo_exit_block(); - DepthFirstTraversal(*exit_block, predecessor, [](cbb_ptr) {}, + DepthFirstTraversal(pseudo_exit, augmented_predecessor_fn, [](cbb_ptr) {}, [&](cbb_ptr b) { postdom_postorder.push_back(b); }, [&](cbb_ptr, cbb_ptr) {}); - auto postdom_edges = - libspirv::CalculateDominators(postdom_postorder, successor); - libspirv::UpdateImmediateDominators( - postdom_edges, [](bb_ptr block, bb_ptr dominator) { - block->SetImmediatePostDominator(dominator); - }); + auto postdom_edges = libspirv::CalculateDominators( + postdom_postorder, augmented_successor_fn); + for (auto edge : postdom_edges) { + edge.first->SetImmediatePostDominator(edge.second); + } } UpdateContinueConstructExitBlocks(function, back_edges); @@ -444,9 +464,10 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) { // dominate auto& blocks = function.get_blocks(); if (blocks.empty() == false) { - for (auto block = begin(blocks) + 1; block != end(blocks); block++) { + for (auto block = begin(blocks) + 1; block != end(blocks); ++block) { if (auto idom = (*block)->GetImmediateDominator()) { - if (block == std::find(begin(blocks), block, idom)) { + if (idom != pseudo_entry && + block == std::find(begin(blocks), block, idom)) { return _.diag(SPV_ERROR_INVALID_CFG) << "Block " << _.getIdName((*block)->get_id()) << " appears in the binary before its dominator " diff --git a/test/Validate.CFG.cpp b/test/Validate.CFG.cpp index 5a713c877..40279467e 100644 --- a/test/Validate.CFG.cpp +++ b/test/Validate.CFG.cpp @@ -511,6 +511,7 @@ TEST_P(ValidateCFG, HeaderDoesntDominatesMergeBad) { str += head >> vector({merge, f}); str += f >> merge; str += merge; + str += "OpFunctionEnd\n"; CompileSuccessfully(str); @@ -679,6 +680,7 @@ TEST_P(ValidateCFG, NestedLoops) { str += loop2_merge >> loop1; str += loop1_merge >> exit; str += exit; + str += "OpFunctionEnd"; CompileSuccessfully(str); ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()); diff --git a/test/Validate.Layout.cpp b/test/Validate.Layout.cpp index f647490f8..05c55a29a 100644 --- a/test/Validate.Layout.cpp +++ b/test/Validate.Layout.cpp @@ -332,6 +332,38 @@ TEST_F(ValidateLayout, InstructionAppearBeforeFunctionDefinition) { EXPECT_THAT(getDiagnosticString(), StrEq("Undef must appear in a block")); } +TEST_F(ValidateLayout, MissingFunctionEndForFunctionWithBody) { + const auto s = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +%void = OpTypeVoid +%tf = OpTypeFunction %void +%f = OpFunction %void None %tf +%l = OpLabel +OpReturn +)"; + + CompileSuccessfully(s); + ASSERT_EQ(SPV_ERROR_INVALID_LAYOUT, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + StrEq("Missing OpFunctionEnd at end of module.")); +} + +TEST_F(ValidateLayout, MissingFunctionEndForFunctionPrototype) { + const auto s = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +%void = OpTypeVoid +%tf = OpTypeFunction %void +%f = OpFunction %void None %tf +)"; + + CompileSuccessfully(s); + ASSERT_EQ(SPV_ERROR_INVALID_LAYOUT, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + StrEq("Missing OpFunctionEnd at end of module.")); +} + using ValidateOpFunctionParameter = spvtest::ValidateBase; TEST_F(ValidateOpFunctionParameter, OpLineBetweenParameters) {