From 4007733c9dc6e3a722f617aa6d0758aaba78a93e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 10 Sep 2024 11:55:00 -0400 Subject: [PATCH] Cleanup `BasicBlockBuilder::make_block()` --- assembly/src/assembler/basic_block_builder.rs | 58 +++++++++++----- assembly/src/assembler/mod.rs | 67 ++++++------------- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/assembly/src/assembler/basic_block_builder.rs b/assembly/src/assembler/basic_block_builder.rs index f13cd024d..1113e1fea 100644 --- a/assembly/src/assembler/basic_block_builder.rs +++ b/assembly/src/assembler/basic_block_builder.rs @@ -161,27 +161,28 @@ impl<'a> BasicBlockBuilder<'a> { /// Span Constructors impl<'a> BasicBlockBuilder<'a> { /// Creates and returns a new basic block node from the operations and decorators currently in - /// this builder. If there are no operations however, we return the decorators that were - /// accumulated up until this point. If the builder is empty, then no node is created and - /// `Nothing` is returned. + /// this builder. /// - /// This consumes all operations and decorators in the builder, but does not touch the - /// operations in the epilogue of the builder. - pub fn make_basic_block(&mut self) -> Result { + /// If there are no operations however, then no node is created, the decorators are left + /// untouched and `None` is returned. Use [`Self::drain_decorators`] to retrieve the decorators + /// in this case. + /// + /// This consumes all operations in the builder, but does not touch the operations in the + /// epilogue of the builder. + pub fn make_basic_block(&mut self) -> Result, AssemblyError> { if !self.ops.is_empty() { let ops = self.ops.drain(..).collect(); - let decorators = self.decorators.drain(..).collect(); + let decorators = if !self.decorators.is_empty() { + Some(self.decorators.drain(..).collect()) + } else { + None + }; - let basic_block_node_id = - self.mast_forest_builder.ensure_block(ops, Some(decorators))?; + let basic_block_node_id = self.mast_forest_builder.ensure_block(ops, decorators)?; - Ok(BasicBlockOrDecorators::BasicBlock(basic_block_node_id)) - } else if !self.decorators.is_empty() { - Ok(BasicBlockOrDecorators::Decorators( - self.decorators.drain(..).map(|(_, decorator_id)| decorator_id).collect(), - )) + Ok(Some(basic_block_node_id)) } else { - Ok(BasicBlockOrDecorators::Nothing) + Ok(None) } } @@ -194,9 +195,34 @@ impl<'a> BasicBlockBuilder<'a> { /// - Operations contained in the epilogue of the builder are appended to the list of ops which /// go into the new BASIC BLOCK node. /// - The builder is consumed in the process. + /// - Hence, any remaining decorators if no basic block was created are drained and returned. pub fn try_into_basic_block(mut self) -> Result { self.ops.append(&mut self.epilogue); - self.make_basic_block() + + if let Some(basic_block_node_id) = self.make_basic_block()? { + Ok(BasicBlockOrDecorators::BasicBlock(basic_block_node_id)) + } else if let Some(decorator_ids) = self.drain_decorators() { + Ok(BasicBlockOrDecorators::Decorators(decorator_ids)) + } else { + Ok(BasicBlockOrDecorators::Nothing) + } + } + + /// Drains and returns the decorators in the builder, if any. + /// + /// This should only be called after [`Self::make_basic_block`], when no blocks were created. + /// In other words, there MUST NOT be any operations left in the builder when this is called. + /// + /// # Panics + /// + /// Panics if there are still operations left in the builder. + pub fn drain_decorators(&mut self) -> Option> { + assert!(self.ops.is_empty()); + if !self.decorators.is_empty() { + Some(self.decorators.drain(..).map(|(_, decorator_id)| decorator_id).collect()) + } else { + None + } } } diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 76acfa47b..6575056e9 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -610,16 +610,12 @@ impl Assembler { if let Some(node_id) = self.compile_instruction(inst, &mut block_builder, proc_ctx)? { - match block_builder.make_basic_block()? { - BasicBlockOrDecorators::BasicBlock(basic_block_id) => { - body_node_ids.push(basic_block_id); - }, - BasicBlockOrDecorators::Decorators(decorator_ids) => { - block_builder - .mast_forest_builder_mut() - .set_before_enter(node_id, decorator_ids); - }, - BasicBlockOrDecorators::Nothing => (), + if let Some(basic_block_id) = block_builder.make_basic_block()? { + body_node_ids.push(basic_block_id); + } else if let Some(decorator_ids) = block_builder.drain_decorators() { + block_builder + .mast_forest_builder_mut() + .set_before_enter(node_id, decorator_ids); } body_node_ids.push(node_id); @@ -627,16 +623,9 @@ impl Assembler { }, Op::If { then_blk, else_blk, .. } => { - let maybe_pre_decorators: Option> = match block_builder - .make_basic_block()? - { - BasicBlockOrDecorators::BasicBlock(basic_block_id) => { - body_node_ids.push(basic_block_id); - None - }, - BasicBlockOrDecorators::Decorators(decorator_ids) => Some(decorator_ids), - BasicBlockOrDecorators::Nothing => None, - }; + if let Some(basic_block_id) = block_builder.make_basic_block()? { + body_node_ids.push(basic_block_id); + } let then_blk = self.compile_body( then_blk.iter(), @@ -653,26 +642,19 @@ impl Assembler { let split_node_id = block_builder.mast_forest_builder_mut().ensure_split(then_blk, else_blk)?; - if let Some(pre_decorator_ids) = maybe_pre_decorators { + if let Some(decorator_ids) = block_builder.drain_decorators() { block_builder .mast_forest_builder_mut() - .set_before_enter(split_node_id, pre_decorator_ids) + .set_before_enter(split_node_id, decorator_ids) } body_node_ids.push(split_node_id); }, Op::Repeat { count, body, .. } => { - let maybe_pre_decorators: Option> = match block_builder - .make_basic_block()? - { - BasicBlockOrDecorators::BasicBlock(basic_block_id) => { - body_node_ids.push(basic_block_id); - None - }, - BasicBlockOrDecorators::Decorators(decorator_ids) => Some(decorator_ids), - BasicBlockOrDecorators::Nothing => None, - }; + if let Some(basic_block_id) = block_builder.make_basic_block()? { + body_node_ids.push(basic_block_id); + } let repeat_node_id = self.compile_body( body.iter(), @@ -681,11 +663,11 @@ impl Assembler { block_builder.mast_forest_builder_mut(), )?; - if let Some(pre_decorators) = maybe_pre_decorators { + if let Some(decorator_ids) = block_builder.drain_decorators() { // Attach the decorators before the first instance of the repeated node let mut first_repeat_node = block_builder.mast_forest_builder_mut()[repeat_node_id].clone(); - first_repeat_node.set_before_enter(pre_decorators); + first_repeat_node.set_before_enter(decorator_ids); let first_repeat_node_id = block_builder .mast_forest_builder_mut() .ensure_node(first_repeat_node)?; @@ -702,16 +684,9 @@ impl Assembler { }, Op::While { body, .. } => { - let maybe_pre_decorators: Option> = match block_builder - .make_basic_block()? - { - BasicBlockOrDecorators::BasicBlock(basic_block_id) => { - body_node_ids.push(basic_block_id); - None - }, - BasicBlockOrDecorators::Decorators(decorator_ids) => Some(decorator_ids), - BasicBlockOrDecorators::Nothing => None, - }; + if let Some(basic_block_id) = block_builder.make_basic_block()? { + body_node_ids.push(basic_block_id); + } let loop_node_id = { let loop_body_node_id = self.compile_body( @@ -722,10 +697,10 @@ impl Assembler { )?; block_builder.mast_forest_builder_mut().ensure_loop(loop_body_node_id)? }; - if let Some(pre_decorator_ids) = maybe_pre_decorators { + if let Some(decorator_ids) = block_builder.drain_decorators() { block_builder .mast_forest_builder_mut() - .set_before_enter(loop_node_id, pre_decorator_ids) + .set_before_enter(loop_node_id, decorator_ids) } body_node_ids.push(loop_node_id);