From d51a4c1065f2e274763a11d3214de716b1ec6aea Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Fri, 17 Nov 2017 16:03:40 -0800 Subject: [PATCH] Replace FunctionBuilder's Drop impl with a finalize function. (#193) * Replace FunctionBuilder's Drop impl with a finalize function. This has the advantage of not triggering assertion failures in the event of abandoning a partially-built function. It has the disadvantage of requiring users to call finalize() explicitly. --- lib/frontend/src/frontend.rs | 135 +++++++++++++++++++------------- lib/frontend/src/lib.rs | 2 + lib/wasm/src/func_translator.rs | 5 +- 3 files changed, 85 insertions(+), 57 deletions(-) diff --git a/lib/frontend/src/frontend.rs b/lib/frontend/src/frontend.rs index ac86e3d64f..5127270cbd 100644 --- a/lib/frontend/src/frontend.rs +++ b/lib/frontend/src/frontend.rs @@ -8,6 +8,7 @@ use cretonne::ir::function::DisplayFunction; use cretonne::isa::TargetIsa; use ssa::{SSABuilder, SideEffects, Block}; use cretonne::entity::{EntityRef, EntityMap, EntitySet}; +use cretonne::packed_option::PackedOption; /// Structure used for translating a series of functions into Cretonne IL. /// @@ -48,8 +49,28 @@ struct EbbData { } struct Position { - ebb: Ebb, - basic_block: Block, + ebb: PackedOption, + basic_block: PackedOption, +} + +impl Position { + fn at(ebb: Ebb, basic_block: Block) -> Self { + Self { + ebb: PackedOption::from(ebb), + basic_block: PackedOption::from(basic_block), + } + } + + fn default() -> Self { + Self { + ebb: PackedOption::default(), + basic_block: PackedOption::default(), + } + } + + fn is_default(&self) -> bool { + self.ebb.is_none() && self.basic_block.is_none() + } } impl ILBuilder @@ -149,7 +170,7 @@ impl<'short, 'long, Variable> InstBuilderBase<'short> for FuncInstBuilder<'short .filter(|dest_ebb| unique.insert(*dest_ebb)) { self.builder.builder.ssa.declare_ebb_predecessor( dest_ebb, - self.builder.position.basic_block, + self.builder.position.basic_block.unwrap(), inst, ) } @@ -213,10 +234,7 @@ where func: func, srcloc: Default::default(), builder: builder, - position: Position { - ebb: Ebb::new(0), - basic_block: Block::new(0), - }, + position: Position::default(), } } @@ -245,13 +263,12 @@ where /// successor), the block will be declared filled and it will not be possible to append /// instructions to it. pub fn switch_to_block(&mut self, ebb: Ebb) { - if !self.builder.ebbs[self.position.ebb].pristine { - // First we check that the previous block has been filled. - debug_assert!( - self.is_unreachable() || self.builder.ebbs[self.position.ebb].filled, - "you have to fill your block before switching" - ); - } + // First we check that the previous block has been filled. + debug_assert!( + self.position.is_default() || self.is_unreachable() || self.is_pristine() || + self.is_filled(), + "you have to fill your block before switching" + ); // We cannot switch to a filled block debug_assert!( !self.builder.ebbs[ebb].filled, @@ -260,7 +277,7 @@ where let basic_block = self.builder.ssa.header_block(ebb); // Then we change the cursor position. - self.position = Position { ebb, basic_block }; + self.position = Position::at(ebb, basic_block); } /// Declares that all the predecessors of this block are known. @@ -299,7 +316,7 @@ where self.func, var, ty, - self.position.basic_block, + self.position.basic_block.unwrap(), ); self.handle_ssa_side_effects(side_effects); val @@ -311,7 +328,7 @@ where self.builder.ssa.def_var( var, val, - self.position.basic_block, + self.position.basic_block.unwrap(), ); } @@ -354,13 +371,13 @@ where /// Returns an object with the [`InstBuilder`](../cretonne/ir/builder/trait.InstBuilder.html) /// trait that allows to conveniently append an instruction to the current `Ebb` being built. pub fn ins<'short>(&'short mut self) -> FuncInstBuilder<'short, 'a, Variable> { - let ebb = self.position.ebb; + let ebb = self.position.ebb.unwrap(); FuncInstBuilder::new(self, ebb) } /// Make sure that the current EBB is inserted in the layout. pub fn ensure_inserted_ebb(&mut self) { - let ebb = self.position.ebb; + let ebb = self.position.ebb.unwrap(); if self.builder.ebbs[ebb].pristine { if !self.func.layout.is_ebb_inserted(ebb) { self.func.layout.append_ebb(ebb); @@ -382,7 +399,7 @@ where self.ensure_inserted_ebb(); FuncCursor::new(self.func) .with_srcloc(self.srcloc) - .at_bottom(self.position.ebb) + .at_bottom(self.position.ebb.unwrap()) } /// Append parameters to the given `Ebb` corresponding to the function @@ -410,6 +427,33 @@ where self.func.dfg.append_ebb_param(ebb, argtyp.value_type); } } + + /// Declare that translation of the current function is complete. This + /// resets the state of the `FunctionBuilder` in preparation to be used + /// for another function. + pub fn finalize(&mut self) { + // Check that all the `Ebb`s are filled and sealed. + debug_assert!( + self.builder.ebbs.keys().all(|ebb| { + self.builder.ebbs[ebb].pristine || self.builder.ssa.is_sealed(ebb) + }), + "all blocks should be sealed before dropping a FunctionBuilder" + ); + debug_assert!( + self.builder.ebbs.keys().all(|ebb| { + self.builder.ebbs[ebb].pristine || self.builder.ebbs[ebb].filled + }), + "all blocks should be filled before dropping a FunctionBuilder" + ); + + // Clear the state (but preserve the allocated buffers) in preparation + // for translation another function. + self.builder.clear(); + + // Reset srcloc and position to initial states. + self.srcloc = Default::default(); + self.position = Position::default(); + } } /// All the functions documented in the previous block are write-only and help you build a valid @@ -475,22 +519,25 @@ where pub fn is_unreachable(&self) -> bool { let is_entry = match self.func.layout.entry_block() { None => false, - Some(entry) => self.position.ebb == entry, + Some(entry) => self.position.ebb.unwrap() == entry, }; - !is_entry && self.builder.ssa.is_sealed(self.position.ebb) && - self.builder.ssa.predecessors(self.position.ebb).is_empty() + !is_entry && self.builder.ssa.is_sealed(self.position.ebb.unwrap()) && + self.builder + .ssa + .predecessors(self.position.ebb.unwrap()) + .is_empty() } /// Returns `true` if and only if no instructions have been added since the last call to /// `switch_to_block`. pub fn is_pristine(&self) -> bool { - self.builder.ebbs[self.position.ebb].pristine + self.builder.ebbs[self.position.ebb.unwrap()].pristine } /// Returns `true` if and only if a terminator instruction has been inserted since the /// last call to `switch_to_block`. pub fn is_filled(&self) -> bool { - self.builder.ebbs[self.position.ebb].filled + self.builder.ebbs[self.position.ebb.unwrap()].filled } /// Returns a displayable object for the function as it is. @@ -501,51 +548,25 @@ where } } -impl<'a, Variable> Drop for FunctionBuilder<'a, Variable> -where - Variable: EntityRef, -{ - /// When a `FunctionBuilder` goes out of scope, it means that the function is fully built. - fn drop(&mut self) { - // Check that all the `Ebb`s are filled and sealed. - debug_assert!( - self.builder.ebbs.keys().all(|ebb| { - self.builder.ebbs[ebb].pristine || self.builder.ssa.is_sealed(ebb) - }), - "all blocks should be sealed before dropping a FunctionBuilder" - ); - debug_assert!( - self.builder.ebbs.keys().all(|ebb| { - self.builder.ebbs[ebb].pristine || self.builder.ebbs[ebb].filled - }), - "all blocks should be filled before dropping a FunctionBuilder" - ); - - // Clear the state (but preserve the allocated buffers) in preparation - // for translation another function. - self.builder.clear(); - } -} - // Helper functions impl<'a, Variable> FunctionBuilder<'a, Variable> where Variable: EntityRef, { fn move_to_next_basic_block(&mut self) { - self.position.basic_block = self.builder.ssa.declare_ebb_body_block( - self.position.basic_block, - ); + self.position.basic_block = PackedOption::from(self.builder.ssa.declare_ebb_body_block( + self.position.basic_block.unwrap(), + )); } fn fill_current_block(&mut self) { - self.builder.ebbs[self.position.ebb].filled = true; + self.builder.ebbs[self.position.ebb.unwrap()].filled = true; } fn declare_successor(&mut self, dest_ebb: Ebb, jump_inst: Inst) { self.builder.ssa.declare_ebb_predecessor( dest_ebb, - self.position.basic_block, + self.position.basic_block.unwrap(), jump_inst, ); } @@ -668,6 +689,8 @@ mod tests { if lazy_seal { builder.seal_all_blocks(); } + + builder.finalize(); } let flags = settings::Flags::new(&settings::builder()); diff --git a/lib/frontend/src/lib.rs b/lib/frontend/src/lib.rs index ad76797129..12b8c8db73 100644 --- a/lib/frontend/src/lib.rs +++ b/lib/frontend/src/lib.rs @@ -128,6 +128,8 @@ //! } //! builder.ins().jump(block1, &[]); //! builder.seal_block(block1); +//! +//! builder.finalize(); //! } //! //! let flags = settings::Flags::new(&settings::builder()); diff --git a/lib/wasm/src/func_translator.rs b/lib/wasm/src/func_translator.rs index 73aa7b433a..b5407decd9 100644 --- a/lib/wasm/src/func_translator.rs +++ b/lib/wasm/src/func_translator.rs @@ -94,7 +94,10 @@ impl FuncTranslator { self.state.initialize(&builder.func.signature, exit_block); parse_local_decls(&mut reader, &mut builder, num_params)?; - parse_function_body(reader, &mut builder, &mut self.state, environ) + parse_function_body(reader, &mut builder, &mut self.state, environ)?; + + builder.finalize(); + Ok(()) } }