diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index 8c84b7a00ae8..8cde7cbe06ac 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -24,7 +24,7 @@ use cranelift_codegen::packed_option::PackedOption; #[derive(Default)] pub struct FunctionBuilderContext { ssa: SSABuilder, - blocks: SecondaryMap, + status: SecondaryMap, types: SecondaryMap, } @@ -41,20 +41,15 @@ pub struct FunctionBuilder<'a> { position: PackedOption, } -#[derive(Clone, Default)] -struct BlockData { - /// A Block is "pristine" iff no instructions have been added since the last - /// call to `switch_to_block()`. - pristine: bool, - - /// A Block is "filled" iff a terminator instruction has been inserted since - /// the last call to `switch_to_block()`. - /// - /// A filled block cannot be pristine. - filled: bool, - - /// Count of parameters not supplied implicitly by the SSABuilder. - user_param_count: usize, +#[derive(Clone, Default, Eq, PartialEq)] +enum BlockStatus { + /// No instructions have been added. + #[default] + Empty, + /// Some instructions have been added, but no terminator. + Partial, + /// A terminator has been added; no further instructions may be added. + Filled, } impl FunctionBuilderContext { @@ -66,12 +61,12 @@ impl FunctionBuilderContext { fn clear(&mut self) { self.ssa.clear(); - self.blocks.clear(); + self.status.clear(); self.types.clear(); } fn is_empty(&self) -> bool { - self.ssa.is_empty() && self.blocks.is_empty() && self.types.is_empty() + self.ssa.is_empty() && self.status.is_empty() && self.types.is_empty() } } @@ -304,11 +299,6 @@ impl<'a> FunctionBuilder<'a> { pub fn create_block(&mut self) -> Block { let block = self.func.dfg.make_block(); self.func_ctx.ssa.declare_block(block); - self.func_ctx.blocks[block] = BlockData { - filled: false, - pristine: true, - user_param_count: 0, - }; block } @@ -337,13 +327,13 @@ impl<'a> FunctionBuilder<'a> { debug_assert!( self.position.is_none() || self.is_unreachable() - || self.is_pristine() - || self.is_filled(), + || self.is_pristine(self.position.unwrap()) + || self.is_filled(self.position.unwrap()), "you have to fill your block before switching" ); // We cannot switch to a filled block debug_assert!( - !self.func_ctx.blocks[block].filled, + !self.is_filled(block), "you cannot switch to a block which is already filled" ); @@ -393,6 +383,12 @@ impl<'a> FunctionBuilder<'a> { /// Returns the Cranelift IR necessary to use a previously defined user /// variable, returning an error if this is not possible. pub fn try_use_var(&mut self, var: Variable) -> Result { + // Assert that we're about to add instructions to this block using the definition of the + // given variable. ssa.use_var is the only part of this crate which can add block parameters + // behind the caller's back. If we disallow calling append_block_param as soon as use_var is + // called, then we enforce a strict separation between user parameters and SSA parameters. + self.ensure_inserted_block(); + let (val, side_effects) = { let ty = *self .func_ctx @@ -534,14 +530,14 @@ impl<'a> FunctionBuilder<'a> { /// Make sure that the current block is inserted in the layout. pub fn ensure_inserted_block(&mut self) { let block = self.position.unwrap(); - if self.func_ctx.blocks[block].pristine { + if self.is_pristine(block) { if !self.func.layout.is_block_inserted(block) { self.func.layout.append_block(block); } - self.func_ctx.blocks[block].pristine = false; + self.func_ctx.status[block] = BlockStatus::Partial; } else { debug_assert!( - !self.func_ctx.blocks[block].filled, + !self.is_filled(block), "you cannot add an instruction to a block already filled" ); } @@ -569,9 +565,12 @@ impl<'a> FunctionBuilder<'a> { // These parameters count as "user" parameters here because they aren't // inserted by the SSABuilder. - let user_param_count = &mut self.func_ctx.blocks[block].user_param_count; + debug_assert!( + self.is_pristine(block), + "You can't add block parameters after adding any instruction" + ); + for argtyp in &self.func.stencil.signature.params { - *user_param_count += 1; self.func .stencil .dfg @@ -585,9 +584,12 @@ impl<'a> FunctionBuilder<'a> { pub fn append_block_params_for_function_returns(&mut self, block: Block) { // These parameters count as "user" parameters here because they aren't // inserted by the SSABuilder. - let user_param_count = &mut self.func_ctx.blocks[block].user_param_count; + debug_assert!( + self.is_pristine(block), + "You can't add block parameters after adding any instruction" + ); + for argtyp in &self.func.stencil.signature.returns { - *user_param_count += 1; self.func .stencil .dfg @@ -602,17 +604,19 @@ impl<'a> FunctionBuilder<'a> { // Check that all the `Block`s are filled and sealed. #[cfg(debug_assertions)] { - for (block, block_data) in self.func_ctx.blocks.iter() { - assert!( - block_data.pristine || self.func_ctx.ssa.is_sealed(block), - "FunctionBuilder finalized, but block {} is not sealed", - block, - ); - assert!( - block_data.pristine || block_data.filled, - "FunctionBuilder finalized, but block {} is not filled", - block, - ); + for block in self.func_ctx.status.keys() { + if !self.is_pristine(block) { + assert!( + self.func_ctx.ssa.is_sealed(block), + "FunctionBuilder finalized, but block {} is not sealed", + block, + ); + assert!( + self.is_filled(block), + "FunctionBuilder finalized, but block {} is not filled", + block, + ); + } } } @@ -620,7 +624,7 @@ impl<'a> FunctionBuilder<'a> { #[cfg(debug_assertions)] { // Iterate manually to provide more helpful error messages. - for block in self.func_ctx.blocks.keys() { + for block in self.func_ctx.status.keys() { if let Err((inst, msg)) = self.func.is_block_basic(block) { let inst_str = self.func.dfg.display_inst(inst); panic!( @@ -665,14 +669,9 @@ impl<'a> FunctionBuilder<'a> { /// instructions to it, otherwise this could interfere with SSA construction. pub fn append_block_param(&mut self, block: Block, ty: Type) -> Value { debug_assert!( - self.func_ctx.blocks[block].pristine, + self.is_pristine(block), "You can't add block parameters after adding any instruction" ); - debug_assert_eq!( - self.func_ctx.blocks[block].user_param_count, - self.func.dfg.num_block_params(block) - ); - self.func_ctx.blocks[block].user_param_count += 1; self.func.dfg.append_block_param(block, ty) } @@ -714,14 +713,14 @@ impl<'a> FunctionBuilder<'a> { /// 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.func_ctx.blocks[self.position.unwrap()].pristine + fn is_pristine(&self, block: Block) -> bool { + self.func_ctx.status[block] == BlockStatus::Empty } /// 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.func_ctx.blocks[self.position.unwrap()].filled + fn is_filled(&self, block: Block) -> bool { + self.func_ctx.status[block] == BlockStatus::Filled } } @@ -1078,7 +1077,7 @@ fn greatest_divisible_power_of_two(size: u64) -> u64 { impl<'a> FunctionBuilder<'a> { /// A Block is 'filled' when a terminator instruction is present. fn fill_current_block(&mut self) { - self.func_ctx.blocks[self.position.unwrap()].filled = true; + self.func_ctx.status[self.position.unwrap()] = BlockStatus::Filled; } fn declare_successor(&mut self, dest_block: Block, jump_inst: Inst) { @@ -1089,10 +1088,12 @@ impl<'a> FunctionBuilder<'a> { fn handle_ssa_side_effects(&mut self, side_effects: SideEffects) { for split_block in side_effects.split_blocks_created { - self.func_ctx.blocks[split_block].filled = true + self.func_ctx.status[split_block] = BlockStatus::Filled; } for modified_block in side_effects.instructions_added_to_blocks { - self.func_ctx.blocks[modified_block].pristine = false + if self.is_pristine(modified_block) { + self.func_ctx.status[modified_block] = BlockStatus::Partial; + } } } } diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 0c93b74d4c61..05fd7281fc96 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -115,6 +115,9 @@ pub fn translate_operator( return Ok(()); } + // Given that we believe the current block is reachable, the FunctionBuilder ought to agree. + debug_assert!(!builder.is_unreachable()); + // This big match treats all Wasm code operators. match op { /********************************** Locals **************************************** @@ -380,18 +383,16 @@ pub fn translate_operator( Operator::End => { let frame = state.control_stack.pop().unwrap(); let next_block = frame.following_code(); - - if !builder.is_unreachable() || !builder.is_pristine() { - let return_count = frame.num_return_values(); - let return_args = state.peekn_mut(return_count); - canonicalise_then_jump(builder, frame.following_code(), return_args); - // You might expect that if we just finished an `if` block that - // didn't have a corresponding `else` block, then we would clean - // up our duplicate set of parameters that we pushed earlier - // right here. However, we don't have to explicitly do that, - // since we truncate the stack back to the original height - // below. - } + let return_count = frame.num_return_values(); + let return_args = state.peekn_mut(return_count); + + canonicalise_then_jump(builder, next_block, return_args); + // You might expect that if we just finished an `if` block that + // didn't have a corresponding `else` block, then we would clean + // up our duplicate set of parameters that we pushed earlier + // right here. However, we don't have to explicitly do that, + // since we truncate the stack back to the original height + // below. builder.switch_to_block(next_block); builder.seal_block(next_block);