From 4fcdc5894676ef76707a14e20e50e4379ed18e89 Mon Sep 17 00:00:00 2001 From: Alastair Donaldson Date: Mon, 28 Jun 2021 20:00:14 +0100 Subject: [PATCH] Add IsReachable function to IRContext (#4323) There was a lot of code in the codebase that would get the dominator analysis for a function and then use it to check whether a block is reachable. In the fuzzer, a utility method had been introduced to make this more concise, but it was not being used consistently. This change moves the utility method to IRContext, so that it can be used throughout the codebase, and refactors all existing checks for block reachability to use the utility method. --- source/fuzz/available_instructions.cpp | 2 +- source/fuzz/fuzzer_pass.cpp | 4 +-- ...uzzer_pass_propagate_instructions_down.cpp | 3 +- ...fuzzer_pass_push_ids_through_variables.cpp | 2 +- source/fuzz/fuzzer_util.cpp | 29 +++++++------------ source/fuzz/fuzzer_util.h | 5 ---- source/fuzz/transformation_add_dead_block.cpp | 6 ++-- source/fuzz/transformation_add_dead_break.cpp | 2 +- .../fuzz/transformation_add_dead_continue.cpp | 3 +- ...nsformation_flatten_conditional_branch.cpp | 10 +++---- .../fuzz/transformation_outline_function.cpp | 3 +- ...nsformation_propagate_instruction_down.cpp | 10 +++---- ...ransformation_push_id_through_variable.cpp | 2 +- source/opt/block_merge_util.cpp | 4 +-- source/opt/ir_context.cpp | 6 ++++ source/opt/ir_context.h | 6 +++- ...oop_to_selection_reduction_opportunity.cpp | 9 ++---- 17 files changed, 46 insertions(+), 60 deletions(-) diff --git a/source/fuzz/available_instructions.cpp b/source/fuzz/available_instructions.cpp index e25ed900..0db8b207 100644 --- a/source/fuzz/available_instructions.cpp +++ b/source/fuzz/available_instructions.cpp @@ -44,7 +44,7 @@ AvailableInstructions::AvailableInstructions( // Consider every reachable block in the function. auto dominator_analysis = ir_context->GetDominatorAnalysis(&function); for (auto& block : function) { - if (!fuzzerutil::BlockIsReachableInItsFunction(ir_context, &block)) { + if (!ir_context->IsReachable(block)) { // The block is not reachable. continue; } diff --git a/source/fuzz/fuzzer_pass.cpp b/source/fuzz/fuzzer_pass.cpp index f67efc66..9e95190c 100644 --- a/source/fuzz/fuzzer_pass.cpp +++ b/source/fuzz/fuzzer_pass.cpp @@ -111,10 +111,8 @@ void FuzzerPass::ForEachInstructionWithInstructionDescriptor( // module. std::vector reachable_blocks; - const auto* dominator_analysis = - GetIRContext()->GetDominatorAnalysis(function); for (auto& block : *function) { - if (dominator_analysis->IsReachable(&block)) { + if (GetIRContext()->IsReachable(block)) { reachable_blocks.push_back(&block); } } diff --git a/source/fuzz/fuzzer_pass_propagate_instructions_down.cpp b/source/fuzz/fuzzer_pass_propagate_instructions_down.cpp index af27a5da..89e14370 100644 --- a/source/fuzz/fuzzer_pass_propagate_instructions_down.cpp +++ b/source/fuzz/fuzzer_pass_propagate_instructions_down.cpp @@ -31,8 +31,7 @@ void FuzzerPassPropagateInstructionsDown::Apply() { for (const auto& function : *GetIRContext()->module()) { std::vector reachable_blocks; for (const auto& block : function) { - if (GetIRContext()->GetDominatorAnalysis(&function)->IsReachable( - &block)) { + if (GetIRContext()->IsReachable(block)) { reachable_blocks.push_back(&block); } } diff --git a/source/fuzz/fuzzer_pass_push_ids_through_variables.cpp b/source/fuzz/fuzzer_pass_push_ids_through_variables.cpp index 54e589c9..7b436c17 100644 --- a/source/fuzz/fuzzer_pass_push_ids_through_variables.cpp +++ b/source/fuzz/fuzzer_pass_push_ids_through_variables.cpp @@ -47,7 +47,7 @@ void FuzzerPassPushIdsThroughVariables::Apply() { // The block containing the instruction we are going to insert before // must be reachable. - if (!fuzzerutil::BlockIsReachableInItsFunction(GetIRContext(), block)) { + if (!GetIRContext()->IsReachable(*block)) { return; } diff --git a/source/fuzz/fuzzer_util.cpp b/source/fuzz/fuzzer_util.cpp index 08b927e0..4beb67fd 100644 --- a/source/fuzz/fuzzer_util.cpp +++ b/source/fuzz/fuzzer_util.cpp @@ -252,11 +252,11 @@ bool BlockIsBackEdge(opt::IRContext* context, uint32_t block_id, return false; } - // |block_id| must be reachable and be dominated by |loop_header|. + // |block| must be reachable and be dominated by |loop_header|. opt::DominatorAnalysis* dominator_analysis = context->GetDominatorAnalysis(loop_header->GetParent()); - return dominator_analysis->IsReachable(block_id) && - dominator_analysis->Dominates(loop_header_id, block_id); + return context->IsReachable(*block) && + dominator_analysis->Dominates(loop_header, block); } bool BlockIsInLoopContinueConstruct(opt::IRContext* context, uint32_t block_id, @@ -284,13 +284,6 @@ opt::BasicBlock::iterator GetIteratorForInstruction( return block->end(); } -bool BlockIsReachableInItsFunction(opt::IRContext* context, - opt::BasicBlock* bb) { - auto enclosing_function = bb->GetParent(); - return context->GetDominatorAnalysis(enclosing_function) - ->Dominates(enclosing_function->entry().get(), bb); -} - bool CanInsertOpcodeBeforeInstruction( SpvOp opcode, const opt::BasicBlock::iterator& instruction_in_block) { if (instruction_in_block->PreviousNode() && @@ -660,13 +653,12 @@ bool IdIsAvailableAtUse(opt::IRContext* context, // It is not OK for a definition to use itself. return false; } - auto dominator_analysis = context->GetDominatorAnalysis(enclosing_function); - if (!dominator_analysis->IsReachable( - context->get_instr_block(use_instruction)) || - !dominator_analysis->IsReachable(context->get_instr_block(id))) { + if (!context->IsReachable(*context->get_instr_block(use_instruction)) || + !context->IsReachable(*context->get_instr_block(id))) { // Skip unreachable blocks. return false; } + auto dominator_analysis = context->GetDominatorAnalysis(enclosing_function); if (use_instruction->opcode() == SpvOpPhi) { // In the case where the use is an operand to OpPhi, it is actually the // *parent* block associated with the operand that must be dominated by @@ -704,8 +696,8 @@ bool IdIsAvailableBeforeInstruction(opt::IRContext* context, } const auto* dominator_analysis = context->GetDominatorAnalysis(function_enclosing_instruction); - if (dominator_analysis->IsReachable(context->get_instr_block(instruction)) && - dominator_analysis->IsReachable(context->get_instr_block(id)) && + if (context->IsReachable(*context->get_instr_block(instruction)) && + context->IsReachable(*context->get_instr_block(id)) && dominator_analysis->Dominates(id_definition, instruction)) { // The id's definition dominates the instruction, and both the definition // and the instruction are in reachable blocks, thus the id is available at @@ -715,8 +707,7 @@ bool IdIsAvailableBeforeInstruction(opt::IRContext* context, if (id_definition->opcode() == SpvOpVariable && function_enclosing_instruction == context->get_instr_block(id)->GetParent()) { - assert(!dominator_analysis->IsReachable( - context->get_instr_block(instruction)) && + assert(!context->IsReachable(*context->get_instr_block(instruction)) && "If the instruction were in a reachable block we should already " "have returned true."); // The id is a variable and it is in the same function as |instruction|. @@ -1883,7 +1874,7 @@ bool NewTerminatorPreservesDominationRules(opt::IRContext* ir_context, // all its dependencies satisfy domination rules (i.e. all id operands // dominate that instruction). for (const auto& block : *mutated_block->GetParent()) { - if (!dominator_analysis.IsReachable(&block)) { + if (!ir_context->IsReachable(block)) { // If some block is not reachable then we don't need to worry about the // preservation of domination rules for its instructions. continue; diff --git a/source/fuzz/fuzzer_util.h b/source/fuzz/fuzzer_util.h index dd7bd961..b956507a 100644 --- a/source/fuzz/fuzzer_util.h +++ b/source/fuzz/fuzzer_util.h @@ -108,11 +108,6 @@ bool BlockIsInLoopContinueConstruct(opt::IRContext* context, uint32_t block_id, opt::BasicBlock::iterator GetIteratorForInstruction( opt::BasicBlock* block, const opt::Instruction* inst); -// Returns true if and only if there is a path to |bb| from the entry block of -// the function that contains |bb|. -bool BlockIsReachableInItsFunction(opt::IRContext* context, - opt::BasicBlock* bb); - // Determines whether it is OK to insert an instruction with opcode |opcode| // before |instruction_in_block|. bool CanInsertOpcodeBeforeInstruction( diff --git a/source/fuzz/transformation_add_dead_block.cpp b/source/fuzz/transformation_add_dead_block.cpp index 82e8cd8f..df700ce5 100644 --- a/source/fuzz/transformation_add_dead_block.cpp +++ b/source/fuzz/transformation_add_dead_block.cpp @@ -79,9 +79,7 @@ bool TransformationAddDeadBlock::IsApplicable( } // |existing_block| must be reachable. - opt::DominatorAnalysis* dominator_analysis = - ir_context->GetDominatorAnalysis(existing_block->GetParent()); - if (!dominator_analysis->IsReachable(existing_block->id())) { + if (!ir_context->IsReachable(*existing_block)) { return false; } @@ -94,6 +92,8 @@ bool TransformationAddDeadBlock::IsApplicable( // the selection construct, its header |existing_block| will not dominate the // merge block |successor_block_id|, which is invalid. Thus, |existing_block| // must dominate |successor_block_id|. + opt::DominatorAnalysis* dominator_analysis = + ir_context->GetDominatorAnalysis(existing_block->GetParent()); if (!dominator_analysis->Dominates(existing_block->id(), successor_block_id)) { return false; diff --git a/source/fuzz/transformation_add_dead_break.cpp b/source/fuzz/transformation_add_dead_break.cpp index ad46ce7f..32080ca4 100644 --- a/source/fuzz/transformation_add_dead_break.cpp +++ b/source/fuzz/transformation_add_dead_break.cpp @@ -134,7 +134,7 @@ bool TransformationAddDeadBreak::IsApplicable( return false; } - if (!fuzzerutil::BlockIsReachableInItsFunction(ir_context, bb_to)) { + if (!ir_context->IsReachable(*bb_to)) { // If the target of the break is unreachable, we conservatively do not // allow adding a dead break, to avoid the compilations that arise due to // the lack of sensible dominance information for unreachable blocks. diff --git a/source/fuzz/transformation_add_dead_continue.cpp b/source/fuzz/transformation_add_dead_continue.cpp index be6294e8..f2b9ab3f 100644 --- a/source/fuzz/transformation_add_dead_continue.cpp +++ b/source/fuzz/transformation_add_dead_continue.cpp @@ -83,8 +83,7 @@ bool TransformationAddDeadContinue::IsApplicable( auto continue_block = ir_context->cfg()->block(loop_header)->ContinueBlockId(); - if (!fuzzerutil::BlockIsReachableInItsFunction( - ir_context, ir_context->cfg()->block(continue_block))) { + if (!ir_context->IsReachable(*ir_context->cfg()->block(continue_block))) { // If the loop's continue block is unreachable, we conservatively do not // allow adding a dead continue, to avoid the compilations that arise due to // the lack of sensible dominance information for unreachable blocks. diff --git a/source/fuzz/transformation_flatten_conditional_branch.cpp b/source/fuzz/transformation_flatten_conditional_branch.cpp index b8c6de0c..127e7628 100644 --- a/source/fuzz/transformation_flatten_conditional_branch.cpp +++ b/source/fuzz/transformation_flatten_conditional_branch.cpp @@ -441,17 +441,17 @@ bool TransformationFlattenConditionalBranch:: header->terminator()->opcode() == SpvOpBranchConditional && "|header| must be the header of a conditional."); + // |header| must be reachable. + if (!ir_context->IsReachable(*header)) { + return false; + } + auto enclosing_function = header->GetParent(); auto dominator_analysis = ir_context->GetDominatorAnalysis(enclosing_function); auto postdominator_analysis = ir_context->GetPostDominatorAnalysis(enclosing_function); - // |header| must be reachable. - if (!dominator_analysis->IsReachable(header)) { - return false; - } - // Check that the header and the merge block describe a single-entry, // single-exit region. if (!dominator_analysis->Dominates(header->id(), merge_block_id) || diff --git a/source/fuzz/transformation_outline_function.cpp b/source/fuzz/transformation_outline_function.cpp index 84e8ac2c..3140fa6b 100644 --- a/source/fuzz/transformation_outline_function.cpp +++ b/source/fuzz/transformation_outline_function.cpp @@ -180,8 +180,7 @@ bool TransformationOutlineFunction::IsApplicable( // predecessors. If it does, then we do not regard the region as single- // entry-single-exit and hence do not outline it. for (auto pred : ir_context->cfg()->preds(block.id())) { - if (!fuzzerutil::BlockIsReachableInItsFunction( - ir_context, ir_context->cfg()->block(pred))) { + if (!ir_context->IsReachable(*ir_context->cfg()->block(pred))) { // The predecessor is unreachable. return false; } diff --git a/source/fuzz/transformation_propagate_instruction_down.cpp b/source/fuzz/transformation_propagate_instruction_down.cpp index 7713562e..c3b7c4d9 100644 --- a/source/fuzz/transformation_propagate_instruction_down.cpp +++ b/source/fuzz/transformation_propagate_instruction_down.cpp @@ -386,11 +386,8 @@ bool TransformationPropagateInstructionDown::IsApplicableToBlock( return false; } - const auto* dominator_analysis = - ir_context->GetDominatorAnalysis(block->GetParent()); - // |block| must be reachable. - if (!dominator_analysis->IsReachable(block)) { + if (!ir_context->IsReachable(*block)) { return false; } @@ -430,6 +427,9 @@ bool TransformationPropagateInstructionDown::IsApplicableToBlock( auto phi_block_id = GetOpPhiBlockId(ir_context, block_id, *inst_to_propagate, successor_ids); + const auto* dominator_analysis = + ir_context->GetDominatorAnalysis(block->GetParent()); + // Make sure we can adjust all users of the propagated instruction. return ir_context->get_def_use_mgr()->WhileEachUse( inst_to_propagate, @@ -537,7 +537,7 @@ uint32_t TransformationPropagateInstructionDown::GetOpPhiBlockId( // Check that |merge_block_id| is reachable in the CFG and |block_id| // dominates |merge_block_id|. - if (!dominator_analysis->IsReachable(merge_block_id) || + if (!ir_context->IsReachable(*ir_context->cfg()->block(merge_block_id)) || !dominator_analysis->Dominates(block_id, merge_block_id)) { return 0; } diff --git a/source/fuzz/transformation_push_id_through_variable.cpp b/source/fuzz/transformation_push_id_through_variable.cpp index 0df1da6b..ff525165 100644 --- a/source/fuzz/transformation_push_id_through_variable.cpp +++ b/source/fuzz/transformation_push_id_through_variable.cpp @@ -61,7 +61,7 @@ bool TransformationPushIdThroughVariable::IsApplicable( // The instruction to insert before must belong to a reachable block. auto basic_block = ir_context->get_instr_block(instruction_to_insert_before); - if (!fuzzerutil::BlockIsReachableInItsFunction(ir_context, basic_block)) { + if (!ir_context->IsReachable(*basic_block)) { return false; } diff --git a/source/opt/block_merge_util.cpp b/source/opt/block_merge_util.cpp index 14b5d364..39b50742 100644 --- a/source/opt/block_merge_util.cpp +++ b/source/opt/block_merge_util.cpp @@ -104,9 +104,7 @@ bool CanMergeWithSuccessor(IRContext* context, BasicBlock* block) { } // Don't bother trying to merge unreachable blocks. - if (auto dominators = context->GetDominatorAnalysis(block->GetParent())) { - if (!dominators->IsReachable(block)) return false; - } + if (!context->IsReachable(*block)) return false; Instruction* merge_inst = block->GetMergeInst(); const bool pred_is_header = IsHeader(block); diff --git a/source/opt/ir_context.cpp b/source/opt/ir_context.cpp index 03afe6e8..74f38786 100644 --- a/source/opt/ir_context.cpp +++ b/source/opt/ir_context.cpp @@ -1034,5 +1034,11 @@ bool IRContext::CheckCFG() { return true; } + +bool IRContext::IsReachable(const opt::BasicBlock& bb) { + auto enclosing_function = bb.GetParent(); + return GetDominatorAnalysis(enclosing_function) + ->Dominates(enclosing_function->entry().get(), &bb); +} } // namespace opt } // namespace spvtools diff --git a/source/opt/ir_context.h b/source/opt/ir_context.h index aab35162..f5f38a64 100644 --- a/source/opt/ir_context.h +++ b/source/opt/ir_context.h @@ -598,10 +598,14 @@ class IRContext { bool ProcessCallTreeFromRoots(ProcessFunction& pfn, std::queue* roots); - // Emmits a error message to the message consumer indicating the error + // Emits a error message to the message consumer indicating the error // described by |message| occurred in |inst|. void EmitErrorMessage(std::string message, Instruction* inst); + // Returns true if and only if there is a path to |bb| from the entry block of + // the function that contains |bb|. + bool IsReachable(const opt::BasicBlock& bb); + private: // Builds the def-use manager from scratch, even if it was already valid. void BuildDefUseManager() { diff --git a/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp b/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp index 022c978b..850af456 100644 --- a/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp +++ b/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp @@ -27,10 +27,8 @@ const uint32_t kMergeNodeIndex = 0; bool StructuredLoopToSelectionReductionOpportunity::PreconditionHolds() { // Is the loop header reachable? - return loop_construct_header_->GetLabel() - ->context() - ->GetDominatorAnalysis(loop_construct_header_->GetParent()) - ->IsReachable(loop_construct_header_); + return loop_construct_header_->GetLabel()->context()->IsReachable( + *loop_construct_header_); } void StructuredLoopToSelectionReductionOpportunity::Apply() { @@ -78,8 +76,7 @@ void StructuredLoopToSelectionReductionOpportunity::RedirectToClosestMergeBlock( } already_seen.insert(pred); - if (!context_->GetDominatorAnalysis(loop_construct_header_->GetParent()) - ->IsReachable(pred)) { + if (!context_->IsReachable(*context_->cfg()->block(pred))) { // We do not care about unreachable predecessors (and dominance // information, and thus the notion of structured control flow, makes // little sense for unreachable blocks).