Fix infinite loop in dominance calculation.

Ensure the dominance calculation visits all nodes in the CFG.
The successor list of the pseudo-entry node is augmented with
a single node in each cycle that otherwise would not be visited.
Similarly, the predecssors list of the pseduo-exit node is augmented
with the a single node in each cycle that otherwise would not
be visited.

Pulls DepthFirstSearch out so it's accessible outside of the dominator
calculation.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/279
This commit is contained in:
David Neto 2016-07-27 17:02:22 -04:00
parent 74eb532a1d
commit c978b72477
8 changed files with 307 additions and 138 deletions

View File

@ -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

View File

@ -76,14 +76,6 @@ void BasicBlock::RegisterBranchInstruction(SpvOp branch_instruction) {
return;
}
void BasicBlock::SetSuccessorsUnsafe(std::vector<BasicBlock*>&& others) {
successors_ = std::move(others);
}
void BasicBlock::SetPredecessorsUnsafe(std::vector<BasicBlock*>&& others) {
predecessors_ = std::move(others);
}
BasicBlock::DominatorIterator::DominatorIterator() : current_(nullptr) {}
BasicBlock::DominatorIterator::DominatorIterator(

View File

@ -121,14 +121,6 @@ class BasicBlock {
/// Adds @p next BasicBlocks as successors of this BasicBlock
void RegisterSuccessors(const std::vector<BasicBlock*>& next = {});
/// Set the successors to this block, without updating other internal state,
/// and without updating the other blocks.
void SetSuccessorsUnsafe(std::vector<BasicBlock*>&& others);
/// Set the predecessors to this block, without updating other internal state,
/// and without updating the other blocks.
void SetPredecessorsUnsafe(std::vector<BasicBlock*>&& others);
/// Returns true if the id of the BasicBlock matches
bool operator==(const BasicBlock& other) const { return other.id_ == id_; }

View File

@ -29,10 +29,13 @@
#include <cassert>
#include <algorithm>
#include <unordered_set>
#include <unordered_map>
#include <utility>
#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<BasicBlock*> TraversalRoots(const std::vector<BasicBlock*>& 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<const BasicBlock*> 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<BasicBlock*> 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<BasicBlock*> sources;
vector<BasicBlock*> 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<BasicBlock*>& 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<Construct>& Function::constructs() const { return cfg_constructs_; }
list<Construct>& Function::constructs() { return cfg_constructs_; }
@ -253,4 +282,49 @@ pair<BasicBlock*, bool> Function::GetBlock(uint32_t block_id) {
tie(out, defined) = const_cast<const Function*>(this)->GetBlock(block_id);
return make_pair(const_cast<BasicBlock*>(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

View File

@ -28,6 +28,7 @@
#define LIBSPIRV_VAL_FUNCTION_H_
#include <list>
#include <functional>
#include <unordered_map>
#include <unordered_set>
#include <vector>
@ -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<BasicBlock*>* 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<BasicBlock*>* pseudo_exit_blocks() const {
return &pseudo_exit_blocks_;
}
using GetBlocksFunction =
std::function<const std::vector<BasicBlock*>*(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<BasicBlock*> pseudo_entry_blocks_;
// A vector containing pseudo_exit_block_.
const std::vector<BasicBlock*> 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<const BasicBlock*, std::vector<BasicBlock*>>
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<const BasicBlock*, std::vector<BasicBlock*>>
augmented_predecessors_map_;
/// The constructs that are available in this function
std::list<Construct> cfg_constructs_;

View File

@ -57,6 +57,31 @@ class ValidationState_t;
using get_blocks_func =
std::function<const std::vector<BasicBlock*>*(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<void(const BasicBlock*)> preorder,
std::function<void(const BasicBlock*)> postorder,
std::function<void(const BasicBlock*, const BasicBlock*)> backedge);
/// @brief Calculates dominator edges for a set of blocks
///
/// Computes dominators using the algorithm of Cooper, Harvey, and Kennedy

View File

@ -89,26 +89,8 @@ bool FindInWorkList(const vector<block_info>& 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<void(cbb_ptr)> preorder,
@ -146,8 +128,6 @@ void DepthFirstTraversal(const BasicBlock* entry,
}
}
} // namespace
vector<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
const vector<cbb_ptr>& 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<const BasicBlock*> postorder;
vector<const BasicBlock*> postdom_postorder;
vector<pair<uint32_t, uint32_t>> 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())

View File

@ -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<Block>({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");