Use pseudo entry and pseudo exit blocks for dominance.
authorDavid Neto <dneto@google.com>
Fri, 24 Jun 2016 06:14:16 +0000 (02:14 -0400)
committerDavid Neto <dneto@google.com>
Fri, 24 Jun 2016 21:08:20 +0000 (17:08 -0400)
For dominance calculations we use an "augmented" CFG
where we always add a pseudo-entry node that is the predecessor
in the augmented CFG to any nodes that have no predecessors in the
regular CFG.  Similarly, we add a pseudo-exit node that is the
predecessor in the augmented CFG that is a successor to any
node that has no successors in the regular CFG.

Pseudo entry and exit blocks live in the Function object.

Fixes a subtle problem where we were implicitly creating
the block_details for the pseudo-exit node since it didn't
appear in the idoms map, and yet we referenced it.  In such a case the
contents of the block details could be garbage, or zero-initialized.
That sometimes caused incorrect calculation of immediate dominators
and post-dominators.  For example, on a debug build where the details
could be zero-initialized, the dominator of an unreachable block would
be given as the pseudo-exit node.  Bizarre.

Also, enforce the rule that you must have an OpFunctionEnd to close off
the last function.

source/val/BasicBlock.cpp
source/val/BasicBlock.h
source/val/Function.cpp
source/val/Function.h
source/val/ValidationState.cpp
source/validate.cpp
source/validate.h
source/validate_cfg.cpp
test/Validate.CFG.cpp
test/Validate.Layout.cpp

index 4325736..7e66803 100644 (file)
@@ -26,6 +26,7 @@
 
 #include "BasicBlock.h"
 
+#include <utility>
 #include <vector>
 
 using std::vector;
@@ -75,6 +76,14 @@ 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(
index 8818faa..d421ae8 100644 (file)
@@ -123,6 +123,14 @@ 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_; }
 
index d2c89fb..ebb1e9f 100644 (file)
@@ -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<uint32_t> 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<BasicBlock*> sources;
+    vector<BasicBlock*> 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<BasicBlock*>& 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_;
index 4344fe9..5552cfe 100644 (file)
@@ -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<uint32_t> 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<BasicBlock*>* 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<BasicBlock*>* 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<uint32_t, BasicBlock> 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<BasicBlock*> pseudo_entry_blocks_;
+
+  // A vector containing pseudo_exit_block_.
+  const std::vector<BasicBlock*> pseudo_exit_blocks_;
+
   /// The constructs that are available in this function
   std::list<Construct> cfg_constructs_;
 
index cf7193c..d09dbaa 100644 (file)
@@ -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;
 }
index 63f7fe0..6e153be 100644 (file)
@@ -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.
index 74b3506..e24f7b1 100644 (file)
@@ -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<std::pair<BasicBlock*, BasicBlock*>> CalculateDominators(
     const std::vector<const BasicBlock*>& postorder,
     get_blocks_func predecessor_func);
index b687661..2402ec1 100644 (file)
@@ -86,14 +86,15 @@ 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
+/// 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<block_info>& 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<void(cbb_ptr)> preorder,
                          function<void(cbb_ptr)> postorder,
                          function<void(cbb_ptr, cbb_ptr)> backedge) {
-  vector<cbb_ptr> out;
   unordered_set<uint32_t> 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<block_info> 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<BasicBlock*>* successor(const BasicBlock* b) {
-  return b->get_successors();
-}
-
-const vector<BasicBlock*>* predecessor(const BasicBlock* b) {
-  return b->get_predecessors();
-}
-
 }  // namespace
 
+
 vector<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
     const vector<cbb_ptr>& postorder, get_blocks_func predecessor_func) {
   struct block_detail {
@@ -170,21 +163,20 @@ vector<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
   bool changed = true;
   while (changed) {
     changed = false;
-    for (auto b = postorder.rbegin() + 1; b != postorder.rend(); b++) {
-      const vector<BasicBlock*>* predecessors = predecessor_func(*b);
+    for (auto b = postorder.rbegin() + 1; b != postorder.rend(); ++b) {
+      const vector<BasicBlock*>& 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<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
   return out;
 }
 
-void UpdateImmediateDominators(
-    const vector<pair<bb_ptr, bb_ptr>>& dom_edges,
-    function<void(BasicBlock*, BasicBlock*)> 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<const BasicBlock*> postorder;
     vector<const BasicBlock*> postdom_postorder;
     vector<pair<uint32_t, uint32_t>> 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 "
index 5a713c8..4027946 100644 (file)
@@ -511,6 +511,7 @@ TEST_P(ValidateCFG, HeaderDoesntDominatesMergeBad) {
   str += head >> vector<Block>({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());
index f647490..05c55a2 100644 (file)
@@ -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<int>;
 
 TEST_F(ValidateOpFunctionParameter, OpLineBetweenParameters) {