diff --git a/CHANGES b/CHANGES index e52765f13..fc09dff38 100644 --- a/CHANGES +++ b/CHANGES @@ -4,9 +4,13 @@ v2016.2-dev 2016-07-19 - Start v2016.2 - Validator is incomplete - Checks ID use block is dominated by definition block - - Add optimization passes + - Add optimization passes (in API and spirv-opt command) - Strip debug info instructions - Freeze spec constant to their default values + - Fixes bugs: + #270: validator: crash when continue construct is unreachable + #279: validator: infinite loop when analyzing some degenerate control + flow graphs v2016.1 2016-07-19 - Fix https://github.com/KhronosGroup/SPIRV-Tools/issues/261 diff --git a/source/val/BasicBlock.cpp b/source/val/BasicBlock.cpp index eb54f2a06..645b555b4 100644 --- a/source/val/BasicBlock.cpp +++ b/source/val/BasicBlock.cpp @@ -76,14 +76,6 @@ 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 9be655c78..ffba779b8 100644 --- a/source/val/BasicBlock.h +++ b/source/val/BasicBlock.h @@ -121,14 +121,6 @@ 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 036fae82d..8d9442a03 100644 --- a/source/val/Function.cpp +++ b/source/val/Function.cpp @@ -29,10 +29,13 @@ #include #include +#include +#include #include #include "val/BasicBlock.h" #include "val/Construct.h" +#include "validate.h" using std::ignore; using std::list; @@ -41,6 +44,53 @@ using std::pair; using std::tie; using std::vector; +namespace { + +using libspirv::BasicBlock; + +// Computes a minimal set of root nodes required to traverse, in the forward +// direction, the CFG represented by the given vector of blocks, and successor +// and predecessor functions. +std::vector TraversalRoots(const std::vector& blocks, + libspirv::get_blocks_func succ_func, + libspirv::get_blocks_func pred_func) { + // The set of nodes which have been visited from any of the roots so far. + std::unordered_set visited; + + auto mark_visited = [&visited](const BasicBlock* b) { visited.insert(b); }; + auto ignore_block = [](const BasicBlock*) {}; + auto ignore_blocks = [](const BasicBlock*, const BasicBlock*) {}; + + auto traverse_from_root = [&mark_visited, &succ_func, &ignore_block, + &ignore_blocks](const BasicBlock* entry) { + DepthFirstTraversal(entry, succ_func, mark_visited, ignore_block, + ignore_blocks); + }; + + std::vector result; + + // First collect nodes without predecessors. + for (auto block : blocks) { + if (pred_func(block)->empty()) { + assert(visited.count(block) == 0 && "Malformed graph!"); + result.push_back(block); + traverse_from_root(block); + } + } + + // Now collect other stranded nodes. These must be in unreachable cycles. + for (auto block : blocks) { + if (visited.count(block) == 0) { + result.push_back(block); + traverse_from_root(block); + } + } + + return result; +} + +} // anonymous namespace + namespace libspirv { // Universal Limit of ResultID + 1 @@ -59,8 +109,6 @@ Function::Function(uint32_t function_id, uint32_t result_type_id, 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_() {} @@ -176,16 +224,7 @@ 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->predecessors()->empty()) sources.push_back(b); - if (b->successors()->empty()) sinks.push_back(b); - } - pseudo_entry_block_.SetSuccessorsUnsafe(std::move(sources)); - pseudo_exit_block_.SetPredecessorsUnsafe(std::move(sinks)); + ComputeAugmentedCFG(); } } @@ -203,16 +242,6 @@ vector& Function::ordered_blocks() { return ordered_blocks_; } const BasicBlock* Function::current_block() const { return current_block_; } BasicBlock* Function::current_block() { return current_block_; } -BasicBlock* Function::pseudo_entry_block() { return &pseudo_entry_block_; } -const BasicBlock* Function::pseudo_entry_block() const { - return &pseudo_entry_block_; -} - -BasicBlock* Function::pseudo_exit_block() { return &pseudo_exit_block_; } -const BasicBlock* Function::pseudo_exit_block() const { - return &pseudo_exit_block_; -} - const list& Function::constructs() const { return cfg_constructs_; } list& Function::constructs() { return cfg_constructs_; } @@ -253,4 +282,49 @@ pair Function::GetBlock(uint32_t block_id) { tie(out, defined) = const_cast(this)->GetBlock(block_id); return make_pair(const_cast(out), defined); } + +Function::GetBlocksFunction Function::AugmentedCFGSuccessorsFunction() const { + return [this](const BasicBlock* block) { + auto where = augmented_successors_map_.find(block); + return where == augmented_successors_map_.end() ? block->successors() + : &(*where).second; + }; +} + +Function::GetBlocksFunction Function::AugmentedCFGPredecessorsFunction() const { + return [this](const BasicBlock* block) { + auto where = augmented_predecessors_map_.find(block); + return where == augmented_predecessors_map_.end() ? block->predecessors() + : &(*where).second; + }; +} + +void Function::ComputeAugmentedCFG() { + // Compute the successors of the pseudo-entry block, and + // the predecessors of the pseudo exit block. + auto succ_func = [](const BasicBlock* b) { return b->successors(); }; + auto pred_func = [](const BasicBlock* b) { return b->predecessors(); }; + auto sources = TraversalRoots(ordered_blocks_, succ_func, pred_func); + auto sinks = TraversalRoots(ordered_blocks_, pred_func, succ_func); + + // Wire up the pseudo entry block. + augmented_successors_map_[&pseudo_entry_block_] = sources; + for (auto block : sources) { + auto& augmented_preds = augmented_predecessors_map_[block]; + const auto& preds = *block->predecessors(); + augmented_preds.reserve(1 + preds.size()); + augmented_preds.push_back(&pseudo_entry_block_); + augmented_preds.insert(augmented_preds.end(), preds.begin(), preds.end()); + } + + // Wire up the pseudo exit block. + augmented_predecessors_map_[&pseudo_exit_block_] = sinks; + for (auto block : sinks) { + auto& augmented_succ = augmented_successors_map_[block]; + const auto& succ = *block->successors(); + augmented_succ.reserve(1 + succ.size()); + augmented_succ.push_back(&pseudo_exit_block_); + augmented_succ.insert(augmented_succ.end(), succ.begin(), succ.end()); + } +}; } /// namespace libspirv diff --git a/source/val/Function.h b/source/val/Function.h index 7a810e1a9..cdf868ca9 100644 --- a/source/val/Function.h +++ b/source/val/Function.h @@ -28,6 +28,7 @@ #define LIBSPIRV_VAL_FUNCTION_H_ #include +#include #include #include #include @@ -149,31 +150,38 @@ class Function { /// Returns the block that is currently being parsed in the binary const BasicBlock* current_block() const; - /// Returns the pseudo exit block - BasicBlock* pseudo_entry_block(); + // For dominance calculations, we want to analyze all the + // blocks in the function, even in degenerate control flow cases + // including unreachable blocks. We therefore make an "augmented CFG" + // which is the same as the ordinary CFG but adds: + // - A pseudo-entry node. + // - A pseudo-exit node. + // - A minimal set of edges so that a forward traversal from the + // pseudo-entry node will visit all nodes. + // - A minimal set of edges so that a backward traversal from the + // pseudo-exit node will visit all nodes. + // In particular, the pseudo-entry node is the unique source of the + // augmented CFG, and the psueo-exit node is the unique sink of the + // augmented CFG. /// Returns the pseudo exit block - const BasicBlock* pseudo_entry_block() const; + BasicBlock* pseudo_entry_block() { return &pseudo_entry_block_; } /// Returns the pseudo exit block - BasicBlock* pseudo_exit_block(); + const BasicBlock* pseudo_entry_block() const { return &pseudo_entry_block_; } /// Returns the pseudo exit block - const BasicBlock* pseudo_exit_block() const; + BasicBlock* pseudo_exit_block() { return &pseudo_exit_block_; } - /// 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* pseudo_entry_blocks() const { - return &pseudo_entry_blocks_; - } + /// Returns the pseudo exit block + const BasicBlock* pseudo_exit_block() const { return &pseudo_exit_block_; } - /// 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* pseudo_exit_blocks() const { - return &pseudo_exit_blocks_; - } + using GetBlocksFunction = + std::function*(const BasicBlock*)>; + /// Returns the block successors function for the augmented CFG. + GetBlocksFunction AugmentedCFGSuccessorsFunction() const; + /// Returns the block predecessors function for the augmented CFG. + GetBlocksFunction AugmentedCFGPredecessorsFunction() const; /// Prints a GraphViz digraph of the CFG of the current funciton void PrintDotGraph() const; @@ -182,6 +190,10 @@ class Function { void PrintBlocks() const; private: + // Computes the representation of the augmented CFG. + // Populates augmented_successors_map_ and augmented_predecessors_map_. + void ComputeAugmentedCFG(); + /// The result id of the OpLabel that defined this block uint32_t id_; @@ -212,28 +224,43 @@ class Function { /// The block that is currently being parsed BasicBlock* current_block_; - /// 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. + /// A pseudo entry node used in dominance analysis. + /// After the function end has been registered, the successor list of the + /// pseudo entry node is the minimal set of nodes such that all nodes in the + /// CFG can be reached by following successor lists. That is, the successors + /// will be: + /// - Any basic block without predecessors. This includes the entry + /// block to the function. + /// - A single node from each otherwise unreachable cycle in the CFG, if + /// such cycles exist. + /// The pseudo entry node does not appear in the predecessor or successor + /// list of any ordinary block. + /// It has no predecessors. /// 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. + /// A pseudo exit block used in dominance analysis. + /// After the function end has been registered, the predecessor list of the + /// pseudo exit node is the minimal set of nodes such that all nodes in the + /// CFG can be reached by following predecessor lists. That is, the + /// predecessors will be: + /// - Any basic block without successors. This includes any basic block + /// ending with an OpReturn, OpReturnValue or similar instructions. + /// - A single node from each otherwise unreachable cycle in the CFG, if + /// such cycles exist. + /// The pseudo exit node does not appear in the predecessor or successor + /// list of any ordinary block. + /// It has no successors. 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_; + // Maps a block to its successors in the augmented CFG, if that set is + // different from its successors in the ordinary CFG. + std::unordered_map> + augmented_successors_map_; + // Maps a block to its predecessors in the augmented CFG, if that set is + // different from its predecessors in the ordinary CFG. + std::unordered_map> + augmented_predecessors_map_; /// The constructs that are available in this function std::list cfg_constructs_; diff --git a/source/validate.h b/source/validate.h index 225d0c3e1..a4d7d2373 100644 --- a/source/validate.h +++ b/source/validate.h @@ -57,6 +57,31 @@ class ValidationState_t; using get_blocks_func = std::function*(const BasicBlock*)>; +/// @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. +/// +/// @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 +/// CFG following preorder traversal semantics +/// @param[in] postorder A function that will be called for every block in a +/// 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 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, + std::function preorder, + std::function postorder, + std::function backedge); + /// @brief Calculates dominator edges for a set of blocks /// /// Computes dominators using the algorithm of Cooper, Harvey, and Kennedy diff --git a/source/validate_cfg.cpp b/source/validate_cfg.cpp index 054bf047a..3380bd4a8 100644 --- a/source/validate_cfg.cpp +++ b/source/validate_cfg.cpp @@ -89,26 +89,8 @@ 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. -/// -/// @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 -/// CFG following preorder traversal semantics -/// @param[in] postorder A function that will be called for every block in a -/// 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 and predecessor_func each return a pointer to a -/// collection such that iterators to that collection remain valid for the -/// lifetime -/// of the algorithm +} // namespace + void DepthFirstTraversal(const BasicBlock* entry, get_blocks_func successor_func, function preorder, @@ -146,8 +128,6 @@ void DepthFirstTraversal(const BasicBlock* entry, } } -} // namespace - vector> CalculateDominators( const vector& postorder, get_blocks_func predecessor_func) { struct block_detail { @@ -421,67 +401,38 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) { << _.getIdName(function.id()); } - // 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.pseudo_entry_block(); - auto* pseudo_exit = function.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.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.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->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->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. + // + // We want to analyze all the blocks in the function, even in degenerate + // control flow cases including unreachable blocks. So use the augmented + // CFG to ensure we cover all the blocks. vector postorder; vector postdom_postorder; vector> back_edges; if (!function.ordered_blocks().empty()) { /// calculate dominators - DepthFirstTraversal(pseudo_entry, augmented_successor_fn, [](cbb_ptr) {}, + DepthFirstTraversal(function.pseudo_entry_block(), + function.AugmentedCFGSuccessorsFunction(), + [](cbb_ptr) {}, [&](cbb_ptr b) { postorder.push_back(b); }, [&](cbb_ptr from, cbb_ptr to) { back_edges.emplace_back(from->id(), to->id()); }); - auto edges = - libspirv::CalculateDominators(postorder, augmented_predecessor_fn); + auto edges = libspirv::CalculateDominators( + postorder, function.AugmentedCFGPredecessorsFunction()); for (auto edge : edges) { edge.first->SetImmediateDominator(edge.second); } /// calculate post dominators - DepthFirstTraversal(pseudo_exit, augmented_predecessor_fn, [](cbb_ptr) {}, + DepthFirstTraversal(function.pseudo_exit_block(), + function.AugmentedCFGPredecessorsFunction(), + [](cbb_ptr) {}, [&](cbb_ptr b) { postdom_postorder.push_back(b); }, [&](cbb_ptr, cbb_ptr) {}); auto postdom_edges = libspirv::CalculateDominators( - postdom_postorder, augmented_successor_fn); + postdom_postorder, function.AugmentedCFGSuccessorsFunction()); for (auto edge : postdom_edges) { edge.first->SetImmediatePostDominator(edge.second); } @@ -494,7 +445,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) { if (blocks.empty() == false) { for (auto block = begin(blocks) + 1; block != end(blocks); ++block) { if (auto idom = (*block)->immediate_dominator()) { - if (idom != pseudo_entry && + if (idom != function.pseudo_entry_block() && block == std::find(begin(blocks), block, idom)) { return _.diag(SPV_ERROR_INVALID_CFG) << "Block " << _.getIdName((*block)->id()) diff --git a/test/Validate.CFG.cpp b/test/Validate.CFG.cpp index 255a29039..378b54480 100644 --- a/test/Validate.CFG.cpp +++ b/test/Validate.CFG.cpp @@ -194,6 +194,87 @@ INSTANTIATE_TEST_CASE_P(StructuredControlFlow, ValidateCFG, ::testing::Values(SpvCapabilityShader, SpvCapabilityKernel)); +TEST_P(ValidateCFG, LoopReachableFromEntryButNeverLeadingToReturn) { + // In this case, the loop is reachable from a node without a predecessor, + // but never reaches a node with a return. + // + // This motivates the need for the pseudo-exit node to have a node + // from a cycle in its predecessors list. Otherwise the validator's + // post-dominance calculation will go into an infinite loop. + // + // For more motivation, see + // https://github.com/KhronosGroup/SPIRV-Tools/issues/279 + string str = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + + OpName %entry "entry" + OpName %loop "loop" + OpName %exit "exit" + +%voidt = OpTypeVoid +%funct = OpTypeFunction %voidt + +%main = OpFunction %voidt None %funct +%entry = OpLabel + OpBranch %loop +%loop = OpLabel + OpLoopMerge %exit %loop None + OpBranch %loop +%exit = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(str); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << str; +} + +TEST_P(ValidateCFG, LoopUnreachableFromEntryButLeadingToReturn) { + // In this case, the loop is not reachable from a node without a + // predecessor, but eventually reaches a node with a return. + // + // This motivates the need for the pseudo-entry node to have a node + // from a cycle in its successors list. Otherwise the validator's + // dominance calculation will go into an infinite loop. + // + // For more motivation, see + // https://github.com/KhronosGroup/SPIRV-Tools/issues/279 + // Before that fix, we'd have an infinite loop when calculating + // post-dominators. + string str = R"( + OpCapability Shader + OpMemoryModel Logical GLSL450 + + OpName %entry "entry" + OpName %loop "loop" + OpName %cont "cont" + OpName %exit "exit" + +%voidt = OpTypeVoid +%funct = OpTypeFunction %voidt +%boolt = OpTypeBool +%false = OpConstantFalse %boolt + +%main = OpFunction %voidt None %funct +%entry = OpLabel + OpReturn + +%loop = OpLabel + OpLoopMerge %exit %cont None + OpBranch %cont + +%cont = OpLabel + OpBranchConditional %false %loop %exit + +%exit = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(str); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << str + << getDiagnosticString(); +} + TEST_P(ValidateCFG, Simple) { bool is_shader = GetParam() == SpvCapabilityShader; Block entry("entry"); @@ -262,6 +343,29 @@ TEST_P(ValidateCFG, VariableNotInFirstBlockBad) { "Variables can only be defined in the first block of a function")); } +TEST_P(ValidateCFG, BlockSelfLoopIsOk) { + bool is_shader = GetParam() == SpvCapabilityShader; + Block entry("entry"); + Block loop("loop", SpvOpBranchConditional); + Block merge("merge", SpvOpReturn); + + entry.SetBody("%cond = OpSLessThan %intt %one %two\n"); + if (is_shader) loop.SetBody("OpLoopMerge %merge %loop None\n"); + + string str = header(GetParam()) + + nameOps("loop", "merge", make_pair("func", "Main")) + + types_consts() + "%func = OpFunction %voidt None %funct\n"; + + str += entry >> loop; + // loop branches to itself, but does not trigger an error. + str += loop >> vector({merge, loop}); + str += merge; + str += "OpFunctionEnd\n"; + + CompileSuccessfully(str); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString(); +} + TEST_P(ValidateCFG, BlockAppearsBeforeDominatorBad) { bool is_shader = GetParam() == SpvCapabilityShader; Block entry("entry");