Avoid checking def-use dominance for OpPhi value operands

The def-use dominance checker doesn't have enough info to know
that a particular use is in an OpPhi, so skip tracking those uses
for now.  Add a TODO to do a proper OpPhi variable-argument check
in the future.

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

diff --git a/source/validate_id.cpp b/source/validate_id.cpp
index 84ba404..7d8c485 100644 (file)
--- a/source/validate_id.cpp
+++ b/source/validate_id.cpp
@@ -2420,6 +2420,9 @@ 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.
   return SPV_SUCCESS;
 }
 
@@ -2450,7 +2453,17 @@ spv_result_t IdPass(ValidationState_t& _,
       case SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID:
       case SPV_OPERAND_TYPE_SCOPE_ID:
         if (_.IsDefinedId(*operand_ptr)) {
-          _.RegisterUseId(*operand_ptr);
+          if (inst->opcode == SpvOpPhi && i > 1) {
+            // For now, ignore uses of IDs as arguments to OpPhi, since
+            // the job of an OpPhi is to allow a block to use an ID from a
+            // block that doesn't dominate the use.
+            // We only track usage by a particular block, rather than
+            // which instruction and operand number is using the value, so
+            // we have to just bluntly avod tracking the use here.
+            // Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/286
+          } else {
+            _.RegisterUseId(*operand_ptr);
+          }
           ret = SPV_SUCCESS;
         } else if (can_have_forward_declared_ids(i)) {
           ret = _.ForwardDeclareId(*operand_ptr);

diff --git a/test/Validate.SSA.cpp b/test/Validate.SSA.cpp
index 3460d92..380bb2f 100644 (file)
--- a/test/Validate.SSA.cpp
+++ b/test/Validate.SSA.cpp
@@ -545,6 +545,7 @@ const string kBasicTypes = R"(
 %zero      = OpConstant %intt 0
 %one       = OpConstant %intt 1
 %ten       = OpConstant %intt 10
+%false     = OpConstantFalse %boolt
 )";
 
 const string kKernelTypesAndConstants = R"(
@@ -1172,6 +1173,34 @@ TEST_F(ValidateSSA, PhiUseDoesntDominateUseOfPhiOperandUsedBeforeDefinitionBad)
     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 +
+               R"(
+%func        = OpFunction %voidt None %vfunct
+%entry       = OpLabel
+               OpBranchConditional %false %if_true %exit
+
+%if_true     = OpLabel
+%copy        = OpCopyObject %boolt %false
+               OpBranch %exit
+
+; The use of %copy here is ok, even though it was defined
+; in a block that does not dominate %exit.  That's the point
+; of an OpPhi.
+%exit        = OpLabel
+%value       = OpPhi %boolt %false %entry %copy %if_true
+               OpReturn
+               OpFunctionEnd
+)";
+
+  CompileSuccessfully(str);
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()) << getDiagnosticString();
+}
+
 TEST_F(ValidateSSA, UseFunctionParameterFromOtherFunctionBad) {
   string str = kHeader +
                "OpName %first \"first\"\n"