Check strict domination of merge block

If a merge block is reachable, then it must be *strictly* dominated
by its header.  Until now we've allowed the header and the merge
block to be the same.

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

Also: Use dominates and postdominates methods on BasicBlock to
improve readability.

CHANGES

@@ -9,6 +9,8 @@ v2016.7-dev 2017-01-06
    #508: Support compilation under CYGWIN
    #517: Fix validation when continue (or case) contstruct is also the head of a
      nested control construct.
+   #551: If a merge block is reachable, it must be *strictly* dominated by its
+     header.
 
 v2016.6 2016-12-13
  - Published the C++ interface for assembling, disassembling, validation, and

source/val/construct.h

@@ -85,6 +85,12 @@ class Construct {
   /// constructs which do not know the back-edge block during construction
   void set_exit(BasicBlock* exit_block);
 
+  // Returns whether the exit block of this construct is the merge block
+  // for an OpLoopMerge or OpSelectionMerge
+  bool ExitBlockIsMergeBlock() const {
+    return type_ == ConstructType::kLoop || type_ == ConstructType::kSelection;
+  }
+
  private:
   /// The type of the construct
   ConstructType type_;

source/validate_cfg.cpp

@@ -278,14 +278,8 @@ tuple<string, string, string> ConstructNames(ConstructType type) {
 string ConstructErrorString(const Construct& construct,
                             const string& header_string,
                             const string& exit_string,
-                            bool post_dominate = false) {
-  string construct_name, header_name, exit_name, dominate_text;
-  if (post_dominate) {
-    dominate_text = "is not post dominated by";
-  } else {
-    dominate_text = "does not dominate";
-  }
-
+                            const string& dominate_text) {
+  string construct_name, header_name, exit_name;
   tie(construct_name, header_name, exit_name) =
       ConstructNames(construct.type());
 
@@ -345,22 +339,29 @@ 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 && 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 the exit block is reachable then it's dominated by the
+    // header.
+    if (merge && merge->reachable()) {
+      if (!header->dominates(*merge)) {
+        return _.diag(SPV_ERROR_INVALID_CFG) << ConstructErrorString(
+                   construct, _.getIdName(header->id()),
+                   _.getIdName(merge->id()), "does not dominate");
+      }
+      // If it's really a merge block for a selection or loop, then it must be
+      // *strictly* dominated by the header.
+      if (construct.ExitBlockIsMergeBlock() && (header == merge)) {
+        return _.diag(SPV_ERROR_INVALID_CFG) << ConstructErrorString(
+                   construct, _.getIdName(header->id()),
+                   _.getIdName(merge->id()), "does not strictly dominate");
+      }
     }
     // 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)
-               << ConstructErrorString(construct, _.getIdName(header->id()),
-                                       _.getIdName(merge->id()), true);
+      if (!merge->postdominates(*header)) {
+        return _.diag(SPV_ERROR_INVALID_CFG) << ConstructErrorString(
+                   construct, _.getIdName(header->id()),
+                   _.getIdName(merge->id()), "is not post dominated by");
       }
     }
     // TODO(umar):  an OpSwitch block dominates all its defined case

test/val/val_cfg_test.cpp

@@ -626,6 +626,38 @@ TEST_P(ValidateCFG, HeaderDoesntDominatesMergeBad) {
   }
 }
 
+TEST_P(ValidateCFG, HeaderDoesntStrictlyDominateMergeBad) {
+  // If a merge block is reachable, then it must be strictly dominated by
+  // its header block.
+  bool is_shader = GetParam() == SpvCapabilityShader;
+  Block head("head", SpvOpBranchConditional);
+  Block exit("exit", SpvOpReturn);
+
+  head.SetBody("%cond = OpSLessThan %intt %one %two\n");
+
+  if (is_shader) head.AppendBody("OpSelectionMerge %head None\n");
+
+  string str = header(GetParam()) +
+               nameOps("head", "exit", make_pair("func", "Main")) +
+               types_consts() + "%func    = OpFunction %voidt None %funct\n";
+
+  str += head >> vector<Block>({exit, exit});
+  str += exit;
+  str += "OpFunctionEnd\n";
+
+  CompileSuccessfully(str);
+  if (is_shader) {
+    ASSERT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
+    EXPECT_THAT(
+        getDiagnosticString(),
+        MatchesRegex("The selection construct with the selection header "
+                     ".\\[head\\] does not strictly dominate the merge block "
+                     ".\\[head\\]"));
+  } else {
+    ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << str;
+  }
+}
+
 TEST_P(ValidateCFG, UnreachableMerge) {
   bool is_shader = GetParam() == SpvCapabilityShader;
   Block entry("entry");