Check dominance of OpPhi parent and variable pairs

diff --git a/source/validate_id.cpp b/source/validate_id.cpp
index 0a50c27..b2bf369 100644 (file)
--- a/source/validate_id.cpp
+++ b/source/validate_id.cpp
@@ -2405,6 +2405,7 @@ spv_result_t UpdateIdUse(ValidationState_t& _) {
 /// NOTE: This function does NOT check module scoped functions which are
 /// checked during the initial binary parse in the IdPass below
 spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _) {
+  unordered_set<const Instruction*> phi_instructions;
   for (const auto& definition : _.all_definitions()) {
     // Check only those definitions defined in a function
     if (const Function* func = definition.second->function()) {
@@ -2415,10 +2416,10 @@ spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _) {
         for (auto& use_index_pair : definition.second->uses()) {
           const Instruction* use = use_index_pair.first;
           if (const BasicBlock* use_block = use->block()) {
-            if (!use_block->reachable()) continue;
-            if (use->opcode() != SpvOpPhi &&
-                use_block->dom_end() ==
-                    find(use_block->dom_begin(), use_block->dom_end(), block)) {
+            if (use_block->reachable() == false) continue;
+            if (use->opcode() == SpvOpPhi) {
+              phi_instructions.insert(use);
+            } else if (!block->dominates(*use->block())) {
               return _.diag(SPV_ERROR_INVALID_ID)
                      << "ID " << _.getIdName(definition.first)
                      << " defined in block " << _.getIdName(block->id())
@@ -2447,9 +2448,25 @@ spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _) {
     // NOTE: Ids defined outside of functions must appear before they are used
     // This check is being performed in the IdPass function
   }
-  // TODO(dneto): We don't track check of IDs by phi nodes.  We should check
-  // that for each (variable, predecessor) pair in an OpPhi, that the variable
-  // is defined in a block that dominates that predecessor block.
+
+  // Check all OpPhi parent blocks are dominated by the variable's defining
+  // blocks
+  for (const Instruction* phi : phi_instructions) {
+    if (phi->block()->reachable() == false) continue;
+    for (size_t i = 3; i < phi->operands().size(); i += 2) {
+      const Instruction* variable = _.FindDef(phi->words(i));
+      const BasicBlock* parent =
+          phi->function()->GetBlock(phi->words(i + 1)).first;
+      if (variable->block() && !variable->block()->dominates(*parent)) {
+        return _.diag(SPV_ERROR_INVALID_ID)
+               << "In OpPhi instruction " << _.getIdName(phi->id()) << ", ID "
+               << _.getIdName(variable->id())
+               << " definition does not dominate its parent "
+               << _.getIdName(parent->id());
+      }
+    }
+  }
+
   return SPV_SUCCESS;
 }
 

diff --git a/test/Validate.SSA.cpp b/test/Validate.SSA.cpp
index 04201be..a84ea84 100644 (file)
--- a/test/Validate.SSA.cpp
+++ b/test/Validate.SSA.cpp
@@ -1140,10 +1140,9 @@ TEST_F(ValidateSSA, PhiUseDoesntDominateDefinitionGood) {
   ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
-TEST_F(ValidateSSA, PhiUseDoesntDominateUseOfPhiOperandUsedBeforeDefinitionBad) {
-  string str = kHeader
-      + "OpName %inew \"inew\""
-      + kBasicTypes +
+TEST_F(ValidateSSA,
+       PhiUseDoesntDominateUseOfPhiOperandUsedBeforeDefinitionBad) {
+  string str = kHeader + "OpName %inew \"inew\"" + kBasicTypes +
                R"(
 %func        = OpFunction %voidt None %vfunct
 %entry       = OpLabel
@@ -1168,17 +1167,14 @@ TEST_F(ValidateSSA, PhiUseDoesntDominateUseOfPhiOperandUsedBeforeDefinitionBad)
 
   CompileSuccessfully(str);
   ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-  EXPECT_THAT(
-    getDiagnosticString(),
-    MatchesRegex("ID .\\[inew\\] has not been defined"));
+  EXPECT_THAT(getDiagnosticString(),
+              MatchesRegex("ID .\\[inew\\] has not been defined"));
 }
 
 TEST_F(ValidateSSA, PhiUseMayComeFromNonDominatingBlockGood) {
-  string str = kHeader
-      + "OpName %if_true \"if_true\"\n"
-      + "OpName %exit \"exit\"\n"
-      + "OpName %copy \"copy\"\n"
-      + kBasicTypes +
+  string str = kHeader + "OpName %if_true \"if_true\"\n" +
+               "OpName %exit \"exit\"\n" + "OpName %copy \"copy\"\n" +
+               kBasicTypes +
                R"(
 %func        = OpFunction %voidt None %vfunct
 %entry       = OpLabel
@@ -1201,14 +1197,11 @@ TEST_F(ValidateSSA, PhiUseMayComeFromNonDominatingBlockGood) {
   ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString();
 }
 
-TEST_F(ValidateSSA, DISABLED_PhiVariableDefMustComeFromBlockDominatingThePredecessorBad) {
-  string str = kHeader
-      + "OpName %if_true \"if_true\"\n"
-      + "OpName %if_false \"if_false\"\n"
-      + "OpName %exit \"exit\"\n"
-      + "OpName %true_copy \"true_copy\"\n"
-      + "OpName %false_copy \"false_copy\"\n"
-      + kBasicTypes +
+TEST_F(ValidateSSA, PhiVariableDefNotDominatedByParentBlockBad) {
+  string str = kHeader + "OpName %if_true \"if_true\"\n" +
+               "OpName %if_false \"if_false\"\n" + "OpName %exit \"exit\"\n" +
+               "OpName %value \"phi\"\n" + "OpName %true_copy \"true_copy\"\n" +
+               "OpName %false_copy \"false_copy\"\n" + kBasicTypes +
                R"(
 %func        = OpFunction %voidt None %vfunct
 %entry       = OpLabel
@@ -1231,7 +1224,41 @@ TEST_F(ValidateSSA, DISABLED_PhiVariableDefMustComeFromBlockDominatingThePredece
 
   CompileSuccessfully(str);
   ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-  // TODO(dneto): Check for a good error message
+  EXPECT_THAT(
+      getDiagnosticString(),
+      MatchesRegex("In OpPhi instruction .\\[phi\\], ID .\\[true_copy\\] "
+                   "definition does not dominate its parent .\\[if_false\\]"));
+}
+
+TEST_F(ValidateSSA,
+       PhiVariableDefDominatesButNotDefinedInParentBlock) {
+  string str = kHeader + "OpName %if_true \"if_true\"\n" +
+               kBasicTypes +
+               R"(
+%func        = OpFunction %voidt None %vfunct
+%entry       = OpLabel
+               OpBranchConditional %false %if_true %if_false
+
+%if_true     = OpLabel
+%true_copy   = OpCopyObject %boolt %false
+               OpBranch %if_tnext
+%if_tnext    = OpLabel
+               OpBranch %exit
+
+%if_false    = OpLabel
+%false_copy  = OpCopyObject %boolt %false
+               OpBranch %if_fnext
+%if_fnext    = OpLabel
+               OpBranch %exit
+
+%exit        = OpLabel
+%value       = OpPhi %boolt %true_copy %if_tnext %false_copy %if_fnext
+               OpReturn
+               OpFunctionEnd
+)";
+
+  CompileSuccessfully(str);
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
 TEST_F(ValidateSSA,