From: David Neto Date: Fri, 5 Aug 2016 14:09:06 +0000 (-0400) Subject: Fix dominance calculation X-Git-Tag: upstream/2018.6~1171 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3184687714d76d920224e761a05c9598de64ad7f;p=platform%2Fupstream%2FSPIRV-Tools.git 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 --- a/source/validate_cfg.cpp +++ b/source/validate_cfg.cpp @@ -147,10 +147,12 @@ vector> CalculateDominators( changed = false; for (auto b = postorder.rbegin() + 1; b != postorder.rend(); ++b) { const vector& 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> 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> 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 --- 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({inner_head, exit}); + str += inner_head >> vector({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