From 1f2fcddd3963b9c29bf360daf7656c5977c2aadd Mon Sep 17 00:00:00 2001 From: Alastair Donaldson Date: Fri, 13 Nov 2020 17:33:15 +0000 Subject: [PATCH] spirv-opt: Set parent when adding basic block (#4021) Ensures that the parent of a block is set in Function::AddBasicBlock. Removes various now unnecessary calls to BasicBlock::SetParent. Fixes #3912. --- source/fuzz/force_render_red.cpp | 1 - source/fuzz/transformation_add_dead_block.cpp | 1 - source/fuzz/transformation_add_early_terminator_wrapper.cpp | 1 - source/fuzz/transformation_add_function.cpp | 4 +--- source/fuzz/transformation_inline_function.cpp | 1 - source/fuzz/transformation_merge_function_returns.cpp | 1 - source/fuzz/transformation_outline_function.cpp | 3 --- source/opt/basic_block.cpp | 3 ++- source/opt/eliminate_dead_members_pass.cpp | 2 +- source/opt/function.cpp | 1 - source/opt/function.h | 1 + source/opt/inst_buff_addr_check_pass.cpp | 4 ---- source/opt/instrument_pass.cpp | 4 ---- source/opt/loop_peeling.cpp | 1 - source/opt/merge_return_pass.cpp | 3 ++- source/opt/wrap_opkill.cpp | 1 - 16 files changed, 7 insertions(+), 25 deletions(-) diff --git a/source/fuzz/force_render_red.cpp b/source/fuzz/force_render_red.cpp index ed60bd032..fd0587a33 100644 --- a/source/fuzz/force_render_red.cpp +++ b/source/fuzz/force_render_red.cpp @@ -212,7 +212,6 @@ bool ForceRenderRed( auto new_exit_block = MakeUnique(std::move(label)); new_exit_block->AddInstruction(MakeUnique( ir_context.get(), SpvOpReturn, 0, 0, opt::Instruction::OperandList())); - new_exit_block->SetParent(entry_point_function); entry_point_function->AddBasicBlock(std::move(new_exit_block)); } diff --git a/source/fuzz/transformation_add_dead_block.cpp b/source/fuzz/transformation_add_dead_block.cpp index 3d77a8087..5dce35694 100644 --- a/source/fuzz/transformation_add_dead_block.cpp +++ b/source/fuzz/transformation_add_dead_block.cpp @@ -153,7 +153,6 @@ void TransformationAddDeadBlock::Apply( : successor_block_id}}}); // Add the new block to the enclosing function. - new_block->SetParent(enclosing_function); enclosing_function->InsertBasicBlockAfter(std::move(new_block), existing_block); diff --git a/source/fuzz/transformation_add_early_terminator_wrapper.cpp b/source/fuzz/transformation_add_early_terminator_wrapper.cpp index 0aa1214b7..9f86070fa 100644 --- a/source/fuzz/transformation_add_early_terminator_wrapper.cpp +++ b/source/fuzz/transformation_add_early_terminator_wrapper.cpp @@ -84,7 +84,6 @@ void TransformationAddEarlyTerminatorWrapper::Apply( // Add the basic block to the function as the sole block, and add the function // to the module. - basic_block->SetParent(function.get()); function->AddBasicBlock(std::move(basic_block)); function->SetFunctionEnd(MakeUnique( ir_context, SpvOpFunctionEnd, 0, 0, opt::Instruction::OperandList())); diff --git a/source/fuzz/transformation_add_function.cpp b/source/fuzz/transformation_add_function.cpp index 214f741af..9799373dc 100644 --- a/source/fuzz/transformation_add_function.cpp +++ b/source/fuzz/transformation_add_function.cpp @@ -276,12 +276,10 @@ bool TransformationAddFunction::TryToAddFunction( // Invariant: we should always be at a label instruction at this point. assert(message_.instruction(instruction_index).opcode() == SpvOpLabel); - // Make a basic block using the label instruction, with the new function - // as its parent. + // Make a basic block using the label instruction. std::unique_ptr block = MakeUnique(InstructionFromMessage( ir_context, message_.instruction(instruction_index))); - block->SetParent(new_function.get()); // Consider successive instructions until we hit another label or the end // of the function, adding each such instruction to the block. diff --git a/source/fuzz/transformation_inline_function.cpp b/source/fuzz/transformation_inline_function.cpp index f58b12327..f997491d2 100644 --- a/source/fuzz/transformation_inline_function.cpp +++ b/source/fuzz/transformation_inline_function.cpp @@ -183,7 +183,6 @@ void TransformationInlineFunction::Apply( auto* cloned_block = block.Clone(ir_context); cloned_block = caller_function->InsertBasicBlockBefore( std::unique_ptr(cloned_block), successor_block); - cloned_block->SetParent(caller_function); cloned_block->GetLabel()->SetResultId(result_id_map.at(cloned_block->id())); fuzzerutil::UpdateModuleIdBound(ir_context, cloned_block->id()); diff --git a/source/fuzz/transformation_merge_function_returns.cpp b/source/fuzz/transformation_merge_function_returns.cpp index 90578a2a3..c7cb5572d 100644 --- a/source/fuzz/transformation_merge_function_returns.cpp +++ b/source/fuzz/transformation_merge_function_returns.cpp @@ -579,7 +579,6 @@ void TransformationMergeFunctionReturns::Apply( } // Insert the new return block at the end of the function. - outer_return_block->SetParent(function); function->AddBasicBlock(std::move(outer_return_block)); // All analyses must be invalidated because the structure of the module was diff --git a/source/fuzz/transformation_outline_function.cpp b/source/fuzz/transformation_outline_function.cpp index 26f9e0b26..643fd6995 100644 --- a/source/fuzz/transformation_outline_function.cpp +++ b/source/fuzz/transformation_outline_function.cpp @@ -778,7 +778,6 @@ void TransformationOutlineFunction::PopulateOutlinedFunction( MakeUnique(MakeUnique( ir_context, SpvOpLabel, 0, message_.new_function_region_entry_block(), opt::Instruction::OperandList())); - outlined_region_entry_block->SetParent(outlined_function); if (&original_region_entry_block == &original_region_exit_block) { outlined_region_exit_block = outlined_region_entry_block.get(); @@ -815,8 +814,6 @@ void TransformationOutlineFunction::PopulateOutlinedFunction( outlined_region_exit_block = cloned_block.get(); } - cloned_block->SetParent(outlined_function); - // Redirect any OpPhi operands whose predecessors are the original region // entry block to become the new function entry block. cloned_block->ForEachPhiInst([this](opt::Instruction* phi_inst) { diff --git a/source/opt/basic_block.cpp b/source/opt/basic_block.cpp index 3608448ba..b7e122c41 100644 --- a/source/opt/basic_block.cpp +++ b/source/opt/basic_block.cpp @@ -248,7 +248,8 @@ BasicBlock* BasicBlock::SplitBasicBlock(IRContext* context, uint32_t label_id, function_->InsertBasicBlockAfter(std::move(new_block_temp), this); new_block->insts_.Splice(new_block->end(), &insts_, iter, end()); - new_block->SetParent(GetParent()); + assert(new_block->GetParent() == GetParent() && + "The parent should already be set appropriately."); context->AnalyzeDefUse(new_block->GetLabelInst()); diff --git a/source/opt/eliminate_dead_members_pass.cpp b/source/opt/eliminate_dead_members_pass.cpp index 5b8f4ec54..ab28932fe 100644 --- a/source/opt/eliminate_dead_members_pass.cpp +++ b/source/opt/eliminate_dead_members_pass.cpp @@ -20,7 +20,7 @@ namespace { const uint32_t kRemovedMember = 0xFFFFFFFF; const uint32_t kSpecConstOpOpcodeIdx = 0; -} +} // namespace namespace spvtools { namespace opt { diff --git a/source/opt/function.cpp b/source/opt/function.cpp index 52054eae7..38c669510 100644 --- a/source/opt/function.cpp +++ b/source/opt/function.cpp @@ -42,7 +42,6 @@ Function* Function::Clone(IRContext* ctx) const { clone->blocks_.reserve(blocks_.size()); for (const auto& b : blocks_) { std::unique_ptr bb(b->Clone(ctx)); - bb->SetParent(clone); clone->AddBasicBlock(std::move(bb)); } diff --git a/source/opt/function.h b/source/opt/function.h index 4b20dcb9b..9e1c72746 100644 --- a/source/opt/function.h +++ b/source/opt/function.h @@ -213,6 +213,7 @@ inline void Function::AddBasicBlock(std::unique_ptr b) { inline void Function::AddBasicBlock(std::unique_ptr b, iterator ip) { + b->SetParent(this); ip.InsertBefore(std::move(b)); } diff --git a/source/opt/inst_buff_addr_check_pass.cpp b/source/opt/inst_buff_addr_check_pass.cpp index fa6c2c6a0..06acc7ea5 100644 --- a/source/opt/inst_buff_addr_check_pass.cpp +++ b/source/opt/inst_buff_addr_check_pass.cpp @@ -202,7 +202,6 @@ uint32_t InstBuffAddrCheckPass::GetSearchAndTestFuncId() { (void)builder.AddInstruction(MakeUnique( context(), SpvOpBranch, 0, 0, std::initializer_list{{SPV_OPERAND_TYPE_ID, {hdr_blk_id}}})); - first_blk_ptr->SetParent(&*input_func); input_func->AddBasicBlock(std::move(first_blk_ptr)); // Linear search loop header block // TODO(greg-lunarg): Implement binary search @@ -246,7 +245,6 @@ uint32_t InstBuffAddrCheckPass::GetSearchAndTestFuncId() { (void)builder.AddInstruction(MakeUnique( context(), SpvOpBranch, 0, 0, std::initializer_list{{SPV_OPERAND_TYPE_ID, {cont_blk_id}}})); - hdr_blk_ptr->SetParent(&*input_func); input_func->AddBasicBlock(std::move(hdr_blk_ptr)); // Continue/Work Block. Read next buffer pointer and break if greater // than ref_ptr arg. @@ -272,7 +270,6 @@ uint32_t InstBuffAddrCheckPass::GetSearchAndTestFuncId() { (void)builder.AddConditionalBranch(uptr_test_inst->result_id(), bound_test_blk_id, hdr_blk_id, kInvalidId, SpvSelectionControlMaskNone); - cont_blk_ptr->SetParent(&*input_func); input_func->AddBasicBlock(std::move(cont_blk_ptr)); // Bounds test block. Read length of selected buffer and test that // all len arg bytes are in buffer. @@ -333,7 +330,6 @@ uint32_t InstBuffAddrCheckPass::GetSearchAndTestFuncId() { std::initializer_list{ {SPV_OPERAND_TYPE_ID, {len_test_inst->result_id()}}})); // Close block - bound_test_blk_ptr->SetParent(&*input_func); input_func->AddBasicBlock(std::move(bound_test_blk_ptr)); // Close function and add function to module std::unique_ptr func_end_inst( diff --git a/source/opt/instrument_pass.cpp b/source/opt/instrument_pass.cpp index e7d97789c..ed34fb025 100644 --- a/source/opt/instrument_pass.cpp +++ b/source/opt/instrument_pass.cpp @@ -758,7 +758,6 @@ uint32_t InstrumentPass::GetStreamWriteFunctionId(uint32_t stage_idx, write_blk_id, merge_blk_id, merge_blk_id, SpvSelectionControlMaskNone); // Close safety test block and gen write block - new_blk_ptr->SetParent(&*output_func); output_func->AddBasicBlock(std::move(new_blk_ptr)); new_blk_ptr = MakeUnique(std::move(write_label)); builder.SetInsertPoint(&*new_blk_ptr); @@ -773,13 +772,11 @@ uint32_t InstrumentPass::GetStreamWriteFunctionId(uint32_t stage_idx, } // Close write block and gen merge block (void)builder.AddBranch(merge_blk_id); - new_blk_ptr->SetParent(&*output_func); output_func->AddBasicBlock(std::move(new_blk_ptr)); new_blk_ptr = MakeUnique(std::move(merge_label)); builder.SetInsertPoint(&*new_blk_ptr); // Close merge block and function and add function to module (void)builder.AddNullaryOp(0, SpvOpReturn); - new_blk_ptr->SetParent(&*output_func); output_func->AddBasicBlock(std::move(new_blk_ptr)); std::unique_ptr func_end_inst( new Instruction(get_module()->context(), SpvOpFunctionEnd, 0, 0, {})); @@ -860,7 +857,6 @@ uint32_t InstrumentPass::GetDirectReadFunctionId(uint32_t param_cnt) { context(), SpvOpReturnValue, 0, 0, std::initializer_list{{SPV_OPERAND_TYPE_ID, {last_value_id}}})); // Close block and function and add function to module - new_blk_ptr->SetParent(&*input_func); input_func->AddBasicBlock(std::move(new_blk_ptr)); std::unique_ptr func_end_inst( new Instruction(get_module()->context(), SpvOpFunctionEnd, 0, 0, {})); diff --git a/source/opt/loop_peeling.cpp b/source/opt/loop_peeling.cpp index 071c27cb1..34f0a8d26 100644 --- a/source/opt/loop_peeling.cpp +++ b/source/opt/loop_peeling.cpp @@ -351,7 +351,6 @@ BasicBlock* LoopPeeling::CreateBlockBefore(BasicBlock* bb) { std::unique_ptr new_bb = MakeUnique(std::unique_ptr(new Instruction( context_, SpvOpLabel, 0, context_->TakeNextId(), {}))); - new_bb->SetParent(loop_utils_.GetFunction()); // Update the loop descriptor. Loop* in_loop = (*loop_utils_.GetLoopDescriptor())[bb]; if (in_loop) { diff --git a/source/opt/merge_return_pass.cpp b/source/opt/merge_return_pass.cpp index b43eb3178..f1601049e 100644 --- a/source/opt/merge_return_pass.cpp +++ b/source/opt/merge_return_pass.cpp @@ -182,7 +182,8 @@ void MergeReturnPass::CreateReturnBlock() { context()->AnalyzeDefUse(final_return_block_->GetLabelInst()); context()->set_instr_block(final_return_block_->GetLabelInst(), final_return_block_); - final_return_block_->SetParent(function_); + assert(final_return_block_->GetParent() == function_ && + "The function should have been set when the block was created."); } void MergeReturnPass::CreateReturn(BasicBlock* block) { diff --git a/source/opt/wrap_opkill.cpp b/source/opt/wrap_opkill.cpp index ae1000c7f..51432a73b 100644 --- a/source/opt/wrap_opkill.cpp +++ b/source/opt/wrap_opkill.cpp @@ -164,7 +164,6 @@ uint32_t WrapOpKill::GetKillingFuncId(SpvOp opcode) { bb->AddInstruction(std::move(kill_inst)); // Add the bb to the function - bb->SetParent((*killing_func).get()); (*killing_func)->AddBasicBlock(std::move(bb)); // Add the function to the module.