From 2ac69bf7d952aba132cd887b09b71622a295b88f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 6 Jul 2023 09:24:43 +0900 Subject: [PATCH] [rust] Passes from HIRBuilder This is a start to porting HIRBuilder, with a largely complete implementation of `build()`. Notably this includes all the passes which build() calls, and the helper functions those passes call in turn: ```rust reverse_postorder_blocks(&mut hir); remove_unreachable_for_updates(&mut hir); remove_unreachable_fallthroughs(&mut hir); remove_unreachable_do_while_statements(&mut hir); mark_instruction_ids(&mut hir)?; mark_predecessors(&mut hir); ``` This was pretty straightforward. I ran into one borrow checker issue with (iirc) `remove_unreachable_for_updates` where i needed to simultaneously hold a mutable reference into the HIR (to write the ForTerminal) and also get an immutable reference to check if the update block is reachable. The challenge is this: ```rust for block in hir.blocks.values_mut() { ^^^^^^^^^^^^^^^^^^^^^^^ hir borrowed mutably here if let TerminalValue::ForTerminal(terminal) = &mut block.terminal.value { if let Some(update) = terminal.update { if !hir.blocks.contains(&update) { ^^^^^^^^^^ borrowed immutably here terminal.update = None; ^^^^^^^^ mutable borrow still active here (and also bc of the loop) } } } } ``` I quickly worked around this as we do in the other passes here by first building a set of the block ids contained in the function (so that the `hir.blocks.contains()` call becomes a call to the copied set of block ids). An alternative would be to add a helper function for mutable iteration which takes the desired value out of the data structure so that you can safely mutate it and reference the rest of the HIR. Usage might look like this: ```rust hir.blocks.each_mut(|mut block, hir| { if let TerminalValue::ForTerminal(terminal) = &mut block.terminal.value { if let Some(update) = terminal.update { if !hir.blocks.contains(&update) { terminal.update = None; } } } }) ``` The lambda would receive the current `block` as a mutable reference, and for the duration of the call `hir.blocks[block.id]` would be set to a sentinel value that would crash if accessed. Meanwhile, `hir` would be a readonly reference to the HIR, allowing the lambda to otherwise lookup information on the HIR but not mutate it. This seems...kinda fine? But also not immediately necessary as there's an easy and efficient-enough-workaround for the cases i've encountered so far. --- compiler/forget/Cargo.lock | 2 + compiler/forget/crates/build-hir/Cargo.toml | 4 +- compiler/forget/crates/build-hir/src/build.rs | 28 +++ .../forget/crates/build-hir/src/builder.rs | 190 +++++++++++++++++- compiler/forget/crates/build-hir/src/lib.rs | 2 + compiler/forget/crates/hir/src/basic_block.rs | 10 +- compiler/forget/crates/hir/src/environment.rs | 22 +- compiler/forget/crates/hir/src/function.rs | 10 + compiler/forget/crates/hir/src/id_types.rs | 14 ++ compiler/forget/crates/hir/src/lib.rs | 10 +- compiler/forget/crates/hir/src/terminal.rs | 78 ++++++- 11 files changed, 345 insertions(+), 25 deletions(-) diff --git a/compiler/forget/Cargo.lock b/compiler/forget/Cargo.lock index bc2efbd30a..f1a47e20fb 100644 --- a/compiler/forget/Cargo.lock +++ b/compiler/forget/Cargo.lock @@ -113,7 +113,9 @@ dependencies = [ name = "build-hir" version = "0.1.0" dependencies = [ + "estree", "hir", + "indexmap 2.0.0", ] [[package]] diff --git a/compiler/forget/crates/build-hir/Cargo.toml b/compiler/forget/crates/build-hir/Cargo.toml index a0d7cd5c92..42a3140f51 100644 --- a/compiler/forget/crates/build-hir/Cargo.toml +++ b/compiler/forget/crates/build-hir/Cargo.toml @@ -6,4 +6,6 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -hir = { path = "../hir" } \ No newline at end of file +hir = { path = "../hir" } +estree = { path = "../estree" } +indexmap = "2.0.0" diff --git a/compiler/forget/crates/build-hir/src/build.rs b/compiler/forget/crates/build-hir/src/build.rs index e69de29bb2..a3c860414a 100644 --- a/compiler/forget/crates/build-hir/src/build.rs +++ b/compiler/forget/crates/build-hir/src/build.rs @@ -0,0 +1,28 @@ +use estree::FunctionDeclaration; +use hir::{Environment, Function}; + +use crate::builder::Builder; + +/// Converts a React function in ESTree format into HIR. Returns the HIR +/// if it was constructed sucessfully, otherwise a list of diagnostics +/// if the input could be not be converted to HIR. +/// +/// Failures generally include nonsensical input (`delete 1`) or syntax +/// that is not yet supported. +pub fn build<'a>( + environment: &'a Environment<'a>, + fun: FunctionDeclaration, +) -> Result, Vec<()>> { + let mut builder = Builder::new(environment); + + let body = match builder.build() { + Ok(body) => body, + Err(diagnostic) => return Err(vec![diagnostic]), + }; + + Ok(Function { + body, + is_async: fun.is_async, + is_generator: fun.is_generator, + }) +} diff --git a/compiler/forget/crates/build-hir/src/builder.rs b/compiler/forget/crates/build-hir/src/builder.rs index afa3dc85b8..63fdb092a7 100644 --- a/compiler/forget/crates/build-hir/src/builder.rs +++ b/compiler/forget/crates/build-hir/src/builder.rs @@ -1,6 +1,7 @@ -use std::collections::HashMap; +use std::collections::HashSet; -use hir::{BasicBlock, BlockId, Environment}; +use hir::{BasicBlock, BlockId, Environment, GotoKind, InstructionIdGenerator, TerminalValue, HIR}; +use indexmap::IndexMap; /// Helper struct used when converting from ESTree to HIR. Includes: /// - Variable resolution @@ -12,7 +13,190 @@ use hir::{BasicBlock, BlockId, Environment}; /// labels and variables, and then calling `build()` when the HIR /// is complete. pub struct Builder<'a> { + #[allow(dead_code)] environment: &'a Environment<'a>, - completed: HashMap>, + + completed: IndexMap>, + entry: BlockId, } + +impl<'a> Builder<'a> { + pub(crate) fn new(environment: &'a Environment<'a>) -> Self { + let entry = environment.next_block_id(); + Self { + environment, + completed: Default::default(), + entry, + } + } + + /// Completes the builder and returns the HIR if it was valid, + /// or a Diagnostic if a validation error occured. + /// + /// TODO: refine the type, only invariants should be possible here, + /// not other types of errors + pub(crate) fn build(self) -> Result, Diagnostic> { + let mut hir = HIR { + entry: self.entry, + blocks: self.completed, + }; + + reverse_postorder_blocks(&mut hir); + remove_unreachable_for_updates(&mut hir); + remove_unreachable_fallthroughs(&mut hir); + remove_unreachable_do_while_statements(&mut hir); + mark_instruction_ids(&mut hir)?; + mark_predecessors(&mut hir); + + Ok(hir) + } +} + +/// Modifies the HIR to put the blocks in reverse postorder, with predecessors before +/// successors (except for the case of loops) +fn reverse_postorder_blocks<'a>(hir: &mut HIR<'a>) { + let mut visited = HashSet::::with_capacity(hir.blocks.len()); + let mut postorder = Vec::::with_capacity(hir.blocks.len()); + fn visit<'a>( + block_id: BlockId, + hir: &HIR<'a>, + visited: &mut HashSet, + postorder: &mut Vec, + ) { + if !visited.insert(block_id) { + // already visited + return; + } + let block = hir.block(block_id); + let terminal = &block.terminal; + match &terminal.value { + TerminalValue::IfTerminal(terminal) => { + visit(terminal.alternate, hir, visited, postorder); + visit(terminal.consequent, hir, visited, postorder); + } + TerminalValue::ForTerminal(terminal) => { + visit(terminal.init, hir, visited, postorder); + } + TerminalValue::DoWhileTerminal(terminal) => { + visit(terminal.body, hir, visited, postorder); + } + TerminalValue::GotoTerminal(terminal) => { + visit(terminal.block, hir, visited, postorder); + } + TerminalValue::ReturnTerminal(..) => { /* no-op */ } + } + } + visit(hir.entry, &hir, &mut visited, &mut postorder); + + // NOTE: could consider sorting the blocks in-place by key + let mut blocks = IndexMap::with_capacity(hir.blocks.len()); + for id in postorder.iter().rev().cloned() { + blocks.insert(id, hir.blocks.remove(&id).unwrap()); + } + + hir.blocks = blocks; +} + +/// Prunes ForTerminal.update values (sets to None) if they are unreachable +fn remove_unreachable_for_updates<'a>(hir: &mut HIR<'a>) { + let block_ids: HashSet = hir.blocks.keys().cloned().collect(); + + for block in hir.blocks.values_mut() { + if let TerminalValue::ForTerminal(terminal) = &mut block.terminal.value { + if let Some(update) = terminal.update { + if !block_ids.contains(&update) { + terminal.update = None; + } + } + } + } +} + +/// Prunes unreachable fallthrough values, setting them to None if the referenced +/// block was not otherwise reachable. +fn remove_unreachable_fallthroughs<'a>(hir: &mut HIR<'a>) { + let block_ids: HashSet = hir.blocks.keys().cloned().collect(); + + for block in hir.blocks.values_mut() { + block + .terminal + .value + .map_optional_fallthroughs(|fallthrough| { + if block_ids.contains(&fallthrough) { + Some(fallthrough) + } else { + None + } + }) + } +} + +/// Rewrites DoWhile statements into Gotos if the test block is not reachable +fn remove_unreachable_do_while_statements<'a>(hir: &mut HIR<'a>) { + let block_ids: HashSet = hir.blocks.keys().cloned().collect(); + + for block in hir.blocks.values_mut() { + if let TerminalValue::DoWhileTerminal(terminal) = &mut block.terminal.value { + if !block_ids.contains(&terminal.test) { + block.terminal.value = TerminalValue::GotoTerminal(hir::GotoTerminal { + block: terminal.body, + kind: GotoKind::Break, + }); + } + } + } +} + +/// Updates the instruction ids for all instructions and blocks +/// Relies on the blocks being in reverse postorder to ensure that id ordering is correct +fn mark_instruction_ids<'a>(hir: &mut HIR<'a>) -> Result<(), Diagnostic> { + let mut id_gen = InstructionIdGenerator::new(); + let mut visited = HashSet::<(usize, usize)>::new(); + for (block_ix, block) in hir.blocks.values_mut().enumerate() { + for (instr_ix, instr) in block.instructions.iter_mut().enumerate() { + invariant(!visited.insert((block_ix, instr_ix)), || ())?; + instr.id = id_gen.next(); + } + block.terminal.id = id_gen.next(); + } + Ok(()) +} + +/// Updates the predecessors of each block +fn mark_predecessors<'a>(hir: &mut HIR<'a>) { + for block in hir.blocks.values_mut() { + block.predecessors.clear(); + } + let mut visited = HashSet::::with_capacity(hir.blocks.len()); + fn visit<'a>( + block_id: BlockId, + prev_id: Option, + hir: &mut HIR<'a>, + visited: &mut HashSet, + ) { + let block = hir.block_mut(block_id); + if let Some(prev_id) = prev_id { + block.predecessors.insert(prev_id); + } + if !visited.insert(block_id) { + return; + } + for successor in block.terminal.value.successors() { + visit(successor, Some(block_id), hir, visited) + } + } + visit(hir.entry, None, hir, &mut visited); +} + +fn invariant(cond: bool, _f: F) -> Result<(), Diagnostic> +where + F: FnOnce() -> Diagnostic, +{ + if !cond { + panic!("Oops invariant failed"); + } + Ok(()) +} + +type Diagnostic = (); diff --git a/compiler/forget/crates/build-hir/src/lib.rs b/compiler/forget/crates/build-hir/src/lib.rs index d7945456f5..8cb58b94e5 100644 --- a/compiler/forget/crates/build-hir/src/lib.rs +++ b/compiler/forget/crates/build-hir/src/lib.rs @@ -1,2 +1,4 @@ mod build; mod builder; + +pub use build::build; diff --git a/compiler/forget/crates/hir/src/basic_block.rs b/compiler/forget/crates/hir/src/basic_block.rs index 6288f66641..81d3028f80 100644 --- a/compiler/forget/crates/hir/src/basic_block.rs +++ b/compiler/forget/crates/hir/src/basic_block.rs @@ -1,4 +1,6 @@ -use crate::{id_types::BlockId, Instruction}; +use std::collections::HashSet; + +use crate::{id_types::BlockId, Instruction, Terminal}; /// Represents a sequence of instructions that will always[1] execute /// consecutively. Concretely, a block may have zero or more instructions @@ -18,6 +20,12 @@ pub struct BasicBlock<'a> { /// The ordered instructions in this block pub instructions: bumpalo::collections::Vec<'a, Instruction<'a>>, + + /// The terminal instruction for the block + pub terminal: Terminal<'a>, + + /// The immediate predecessors of this block + pub predecessors: HashSet, } pub enum BlockKind { diff --git a/compiler/forget/crates/hir/src/environment.rs b/compiler/forget/crates/hir/src/environment.rs index 39b5474d5b..357a9a3b13 100644 --- a/compiler/forget/crates/hir/src/environment.rs +++ b/compiler/forget/crates/hir/src/environment.rs @@ -1,3 +1,5 @@ +use std::cell::Cell; + use bumpalo::Bump; use crate::{BlockId, Features, IdentifierId, Registry}; @@ -20,10 +22,10 @@ pub struct Environment<'a> { allocator: &'a Bump, /// The next available block index - next_block_id: BlockId, + next_block_id: Cell, /// The next available identifier id - next_identifier_id: IdentifierId, + next_identifier_id: Cell, } impl<'a> Environment<'a> { @@ -32,8 +34,8 @@ impl<'a> Environment<'a> { allocator, features, registry, - next_block_id: BlockId(0), - next_identifier_id: IdentifierId(0), + next_block_id: Cell::new(BlockId(0)), + next_identifier_id: Cell::new(IdentifierId(0)), } } @@ -43,16 +45,16 @@ impl<'a> Environment<'a> { } /// Get the next available block id - pub fn next_block_id(&mut self) -> BlockId { - let id = self.next_block_id; - self.next_block_id = id.next(); + pub fn next_block_id(&self) -> BlockId { + let id = self.next_block_id.get(); + self.next_block_id.set(id.next()); id } /// Get the next available identifier id - pub fn next_identifier_id(&mut self) -> IdentifierId { - let id = self.next_identifier_id; - self.next_identifier_id = id.next(); + pub fn next_identifier_id(&self) -> IdentifierId { + let id = self.next_identifier_id.get(); + self.next_identifier_id.set(id.next()); id } } diff --git a/compiler/forget/crates/hir/src/function.rs b/compiler/forget/crates/hir/src/function.rs index ba1402845c..c9085f3209 100644 --- a/compiler/forget/crates/hir/src/function.rs +++ b/compiler/forget/crates/hir/src/function.rs @@ -21,3 +21,13 @@ pub struct HIR<'a> { /// but the blocks are in reverse postorder pub blocks: IndexMap>, } + +impl<'a> HIR<'a> { + pub fn block(&self, id: BlockId) -> &BasicBlock<'a> { + self.blocks.get(&id).unwrap() + } + + pub fn block_mut(&mut self, id: BlockId) -> &mut BasicBlock<'a> { + self.blocks.get_mut(&id).unwrap() + } +} diff --git a/compiler/forget/crates/hir/src/id_types.rs b/compiler/forget/crates/hir/src/id_types.rs index 3ef6f70680..c02b60487f 100644 --- a/compiler/forget/crates/hir/src/id_types.rs +++ b/compiler/forget/crates/hir/src/id_types.rs @@ -32,6 +32,20 @@ impl IdentifierId { /// and to reflect that it is applied to terminals as well pub struct InstructionId(pub(crate) u32); +pub struct InstructionIdGenerator(u32); + +impl InstructionIdGenerator { + pub fn new() -> Self { + Self(0) + } + + pub fn next(&mut self) -> InstructionId { + let id = self.0; + self.0 += 1; + InstructionId(id) + } +} + /// Uniquely identifies a reactive scope pub struct ScopeId(pub(crate) u32); diff --git a/compiler/forget/crates/hir/src/lib.rs b/compiler/forget/crates/hir/src/lib.rs index 44a7bbed1d..85146bb1d3 100644 --- a/compiler/forget/crates/hir/src/lib.rs +++ b/compiler/forget/crates/hir/src/lib.rs @@ -8,12 +8,12 @@ mod registry; mod terminal; mod types; -pub use basic_block::BasicBlock; -pub use environment::Environment; -pub use features::Features; -pub use function::Function; +pub use basic_block::*; +pub use environment::*; +pub use features::*; +pub use function::*; pub use id_types::*; -pub use instruction::Instruction; +pub use instruction::*; pub use registry::Registry; pub use terminal::*; pub use types::*; diff --git a/compiler/forget/crates/hir/src/terminal.rs b/compiler/forget/crates/hir/src/terminal.rs index b8c5a1cc65..467fe1af79 100644 --- a/compiler/forget/crates/hir/src/terminal.rs +++ b/compiler/forget/crates/hir/src/terminal.rs @@ -1,3 +1,5 @@ +use std::iter::Successors; + use crate::{instruction::Place, BlockId, InstructionId}; /// Terminals represent statements or expressions that affect control flow, @@ -9,10 +11,10 @@ pub struct Terminal<'a> { pub enum TerminalValue<'a> { // BranchTerminal(BranchTerminal), - // DoWhileTerminal(DoWhileTerminal), + DoWhileTerminal(DoWhileTerminal), // ForOfTerminal(ForOfTerminal), - // ForTerminal(ForTerminal), - // GotoTerminal(GotoTerminal), + ForTerminal(ForTerminal), + GotoTerminal(GotoTerminal), IfTerminal(IfTerminal<'a>), // LabelTerminal(LabelTerminal), // LogicalTerminal(LogicalTerminal), @@ -26,15 +28,81 @@ pub enum TerminalValue<'a> { // WhileTerminal(WhileTerminal), } +impl<'a> TerminalValue<'a> { + pub fn map_optional_fallthroughs(&mut self, f: F) -> () + where + F: Fn(BlockId) -> Option, + { + match self { + Self::IfTerminal(terminal) => { + terminal.fallthrough = match terminal.fallthrough { + Some(fallthrough) => f(fallthrough), + _ => None, + } + } + Self::DoWhileTerminal(DoWhileTerminal { fallthrough, .. }) + | Self::ForTerminal(ForTerminal { fallthrough, .. }) => { + // statically detect if fallthrough is changed to Option so + // that we can update to map the fallthrough w f() + let _: BlockId = *fallthrough; + } + Self::GotoTerminal(_) | Self::ReturnTerminal(_) => {} + } + } + + pub fn successors(&self) -> Vec { + match self { + Self::IfTerminal(terminal) => { + vec![terminal.consequent, terminal.alternate] + } + Self::ForTerminal(terminal) => { + vec![terminal.init] + } + Self::DoWhileTerminal(terminal) => { + vec![terminal.body] + } + Self::GotoTerminal(terminal) => { + vec![terminal.block] + } + Self::ReturnTerminal(_) => { + vec![] + } + } + } +} + +pub struct GotoTerminal { + pub block: BlockId, + pub kind: GotoKind, +} + +#[derive(Clone, Copy)] +pub enum GotoKind { + Break, + Continue, +} + +pub struct DoWhileTerminal { + pub body: BlockId, + pub test: BlockId, + pub fallthrough: BlockId, +} + pub struct IfTerminal<'a> { pub test: Place<'a>, pub consequent: BlockId, pub alternate: BlockId, pub fallthrough: Option, - pub id: InstructionId, } pub struct ReturnTerminal<'a> { pub value: Place<'a>, - pub id: InstructionId, +} + +pub struct ForTerminal { + pub init: BlockId, + pub test: BlockId, + pub update: Option, + pub body: BlockId, + pub fallthrough: BlockId, }