Fix dominance calculation

Fixes dominance calculation when there is a forward arc from an
unreachable block A to a reachable block B.  Before this fix, we would
say that B is not dominated by the graph entry node, and instead say
that the immediate dominator of B is the psuedo-entry node of the
augmented CFG.

The fix:

- Dominance is defined in terms of a traversal from the entry block
  of the CFG.  So the forward DFS should start from the function
  entry block, not the pseudo-entry-block.

- When following edges backward during dominance calculations, only go to
  nodes that are actually reachable in the forward traversal.
  Important: the sense of reachability flips around when computing
  post-dominance.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/297

diff --git a/source/validate_cfg.cpp b/source/validate_cfg.cpp
index 3380bd4..a901d66 100644 (file)
--- a/source/validate_cfg.cpp
+++ b/source/validate_cfg.cpp
@@ -147,10 +147,12 @@ vector<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
     changed = false;
     for (auto b = postorder.rbegin() + 1; b != postorder.rend(); ++b) {
       const vector<BasicBlock*>& predecessors = *predecessor_func(*b);
-      // first processed/reachable predecessor
+      // Find the first processed/reachable predecessor that is reachable
+      // in the forward traversal.
       auto res = find_if(begin(predecessors), end(predecessors),
                          [&idoms, undefined_dom](BasicBlock* pred) {
-                           return idoms[pred].dominator != undefined_dom;
+                           return idoms.count(pred) &&
+                                  idoms[pred].dominator != undefined_dom;
                          });
       if (res == end(predecessors)) continue;
       const BasicBlock* idom = *res;
@@ -159,6 +161,10 @@ vector<pair<BasicBlock*, BasicBlock*>> CalculateDominators(
       // all other predecessors
       for (const auto* p : predecessors) {
         if (idom == p) continue;
+        // Only consider nodes reachable in the forward traversal.
+        // Otherwise the intersection doesn't make sense and will never
+        // terminate.
+        if (!idoms.count(p)) continue;
         if (idoms[p].dominator != undefined_dom) {
           size_t finger1 = idoms[p].postorder_index;
           size_t finger2 = idom_idx;
@@ -332,7 +338,7 @@ spv_result_t StructuredControlFlowChecks(
 
   // Check the loop headers have exactly one back-edge branching to it
   for (BasicBlock* block : function.ordered_blocks()) {
-    if (block->is_type(kBlockTypeLoop) &&
+    if (block->reachable() && block->is_type(kBlockTypeLoop) &&
         loop_headers.count(block->id()) != 1) {
       return _.diag(SPV_ERROR_INVALID_CFG)
              << "Loop with header " + _.getIdName(block->id()) +
@@ -347,7 +353,7 @@ spv_result_t StructuredControlFlowChecks(
     auto header = construct.entry_block();
     auto merge = construct.exit_block();
 
-    if (!merge) {
+    if (header->reachable() && !merge) {
       string construct_name, header_name, exit_name;
       tie(construct_name, header_name, exit_name) =
           ConstructNames(construct.type());
@@ -357,15 +363,17 @@ spv_result_t StructuredControlFlowChecks(
                     exit_name + ". This may be a bug in the validator.";
     }
 
-    // if the merge block is reachable then it's dominated by the header
-    if (merge->reachable() &&
+    // If the merge block is reachable then it's dominated by the header.
+    if (merge && merge->reachable() &&
         find(merge->dom_begin(), merge->dom_end(), header) ==
             merge->dom_end()) {
       return _.diag(SPV_ERROR_INVALID_CFG)
              << ConstructErrorString(construct, _.getIdName(header->id()),
                                      _.getIdName(merge->id()));
     }
-    if (construct.type() == ConstructType::kContinue) {
+    // Check post-dominance for continue constructs.  But dominance and
+    // post-dominance only make sense when the construct is reachable.
+    if (header->reachable() && construct.type() == ConstructType::kContinue) {
       if (find(header->pdom_begin(), header->pdom_end(), merge) ==
           merge->pdom_end()) {
         return _.diag(SPV_ERROR_INVALID_CFG)
@@ -412,7 +420,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) {
     vector<pair<uint32_t, uint32_t>> back_edges;
     if (!function.ordered_blocks().empty()) {
       /// calculate dominators
-      DepthFirstTraversal(function.pseudo_entry_block(),
+      DepthFirstTraversal(function.first_block(),
                           function.AugmentedCFGSuccessorsFunction(),
                           [](cbb_ptr) {},
                           [&](cbb_ptr b) { postorder.push_back(b); },

diff --git a/test/Validate.CFG.cpp b/test/Validate.CFG.cpp
index 378b544..1866bf2 100644 (file)
--- a/test/Validate.CFG.cpp
+++ b/test/Validate.CFG.cpp
@@ -1149,6 +1149,41 @@ TEST_F(ValidateCFG, LoopWithoutBackEdgesBad) {
                            "back-edges but the standard requires exactly one"));
 }
 
+TEST_P(ValidateCFG,
+       NestedConstructWithUnreachableMergeBlockBranchingToOuterMergeBlock) {
+  // Test for https://github.com/KhronosGroup/SPIRV-Tools/issues/297
+  // The nested construct has an unreachable merge block.  In the
+  // augmented CFG that merge block
+  // we still determine that the
+  bool is_shader = GetParam() == SpvCapabilityShader;
+  Block entry("entry", SpvOpBranchConditional);
+  Block inner_head("inner_head", SpvOpBranchConditional);
+  Block inner_true("inner_true", SpvOpReturn);
+  Block inner_false("inner_false", SpvOpReturn);
+  Block inner_merge("inner_merge");
+  Block exit("exit", SpvOpReturn);
+
+  entry.SetBody("%cond    = OpSLessThan %intt %one %two\n");
+  if (is_shader) {
+    entry.AppendBody("OpSelectionMerge %exit None\n");
+    inner_head.SetBody("OpSelectionMerge %inner_merge None\n");
+  }
+
+  string str = header(GetParam()) + nameOps("entry", "inner_merge", "exit") +
+               types_consts() + "%func    = OpFunction %voidt None %funct\n";
+
+  str += entry >> vector<Block>({inner_head, exit});
+  str += inner_head >> vector<Block>({inner_true, inner_false});
+  str += inner_true;
+  str += inner_false;
+  str += inner_merge >> exit;
+  str += exit;
+  str += "OpFunctionEnd";
+
+  CompileSuccessfully(str);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString();
+}
+
 /// TODO(umar): Switch instructions
 /// TODO(umar): Nested CFG constructs
 }  /// namespace