From a045a2991fcebb922dd374be4ff165f60df81a69 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 14:24:47 -0800 Subject: [PATCH 1/7] Move default blocks into jump tables --- cranelift/codegen/meta/src/shared/entities.rs | 10 -- cranelift/codegen/meta/src/shared/formats.rs | 1 - .../codegen/meta/src/shared/instructions.rs | 3 +- cranelift/codegen/src/inst_predicates.rs | 12 +-- cranelift/codegen/src/ir/function.rs | 10 +- cranelift/codegen/src/ir/jumptable.rs | 96 ++++++++++++------- cranelift/codegen/src/isa/aarch64/lower.isle | 2 +- cranelift/codegen/src/isa/aarch64/mod.rs | 4 +- cranelift/codegen/src/isa/riscv64/inst.isle | 2 +- cranelift/codegen/src/isa/s390x/lower.isle | 2 +- cranelift/codegen/src/isa/x64/lower.isle | 2 +- cranelift/codegen/src/isa/x64/mod.rs | 4 +- cranelift/codegen/src/verifier/mod.rs | 24 +---- cranelift/codegen/src/write.rs | 7 +- cranelift/frontend/src/frontend.rs | 5 +- cranelift/frontend/src/ssa.rs | 14 +-- cranelift/frontend/src/switch.rs | 4 +- cranelift/fuzzgen/src/function_generator.rs | 4 +- cranelift/interpreter/src/step.rs | 9 +- cranelift/reader/src/parser.rs | 13 +-- cranelift/wasm/src/code_translator.rs | 15 +-- 21 files changed, 105 insertions(+), 138 deletions(-) diff --git a/cranelift/codegen/meta/src/shared/entities.rs b/cranelift/codegen/meta/src/shared/entities.rs index e713bc92ec10..374e61f4167b 100644 --- a/cranelift/codegen/meta/src/shared/entities.rs +++ b/cranelift/codegen/meta/src/shared/entities.rs @@ -15,10 +15,6 @@ pub(crate) struct EntityRefs { /// This is primarily used in control flow instructions. pub(crate) block_call: OperandKind, - /// A reference to a basic block in the same function. - /// This is primarily used in control flow instructions. - pub(crate) label: OperandKind, - /// A reference to a basic block in the same function, with its arguments provided. /// This is primarily used in control flow instructions. pub(crate) block_then: OperandKind, @@ -63,12 +59,6 @@ impl EntityRefs { "a basic block in the same function, with its arguments provided.", ), - label: new( - "destination", - "ir::Block", - "a basic block in the same function.", - ), - block_then: new( "block_then", "ir::BlockCall", diff --git a/cranelift/codegen/meta/src/shared/formats.rs b/cranelift/codegen/meta/src/shared/formats.rs index b44ecb83e25d..35ea2e7f8ff4 100644 --- a/cranelift/codegen/meta/src/shared/formats.rs +++ b/cranelift/codegen/meta/src/shared/formats.rs @@ -117,7 +117,6 @@ impl Formats { branch_table: Builder::new("BranchTable") .value() - .imm(&entities.label) .imm(&entities.jump_table) .build(), diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs index 014e3b466105..575caa4437a5 100755 --- a/cranelift/codegen/meta/src/shared/instructions.rs +++ b/cranelift/codegen/meta/src/shared/instructions.rs @@ -19,7 +19,6 @@ fn define_control_flow( ) { let block_call = &Operand::new("block_call", &entities.block_call) .with_doc("Destination basic block, with its arguments provided"); - let label = &Operand::new("label", &entities.label).with_doc("Destination basic block"); ig.push( Inst::new( @@ -93,7 +92,7 @@ fn define_control_flow( "#, &formats.branch_table, ) - .operands_in(vec![x, label, JT]) + .operands_in(vec![x, JT]) .branches(), ); } diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs index 96bd63e602a0..2e876e1045fe 100644 --- a/cranelift/codegen/src/inst_predicates.rs +++ b/cranelift/codegen/src/inst_predicates.rs @@ -190,16 +190,14 @@ pub(crate) fn visit_block_succs( visit(inst, block_else.block(&f.dfg.value_lists), false); } - ir::InstructionData::BranchTable { - table, - destination: dest, - .. - } => { + ir::InstructionData::BranchTable { table, .. } => { + let table = &f.stencil.dfg.jump_tables[*table]; + // The default block is reached via a direct conditional branch, // so it is not part of the table. - visit(inst, *dest, false); + visit(inst, table.default_block(), false); - for &dest in f.stencil.dfg.jump_tables[*table].as_slice() { + for &dest in table.table_slice() { visit(inst, dest, true); } } diff --git a/cranelift/codegen/src/ir/function.rs b/cranelift/codegen/src/ir/function.rs index 7e5176c013c9..8a2c3e66cfd1 100644 --- a/cranelift/codegen/src/ir/function.rs +++ b/cranelift/codegen/src/ir/function.rs @@ -295,20 +295,12 @@ impl FunctionStencil { } } - InstructionData::BranchTable { - table, - destination: default_dest, - .. - } => { + InstructionData::BranchTable { table, .. } => { self.dfg.jump_tables[*table].iter_mut().for_each(|entry| { if *entry == old_dest { *entry = new_dest; } }); - - if *default_dest == old_dest { - *default_dest = new_dest; - } } inst => debug_assert!(!inst.opcode().is_branch()), diff --git a/cranelift/codegen/src/ir/jumptable.rs b/cranelift/codegen/src/ir/jumptable.rs index f92ae1162c0f..427bba05168d 100644 --- a/cranelift/codegen/src/ir/jumptable.rs +++ b/cranelift/codegen/src/ir/jumptable.rs @@ -14,6 +14,10 @@ use serde::{Deserialize, Serialize}; /// Contents of a jump table. /// /// All jump tables use 0-based indexing and are densely populated. +/// +/// The default block for the jump table is stored as the last element of the underlying vector, +/// and is not included in the length of the jump table. It can be accessed through the +/// `default_block` function. The default block is iterated via the `iter` and `iter_mut` methods. #[derive(Clone, PartialEq, Hash)] #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))] pub struct JumpTableData { @@ -23,42 +27,59 @@ pub struct JumpTableData { impl JumpTableData { /// Create a new empty jump table. - pub fn new() -> Self { - Self { table: Vec::new() } + pub fn new(def: Block) -> Self { + Self { table: vec![def] } } - /// Create a new empty jump table with the specified capacity. - pub fn with_capacity(capacity: usize) -> Self { - Self { - table: Vec::with_capacity(capacity), - } - } /// Create a new jump table with the provided blocks - pub fn with_blocks(table: Vec) -> Self { + pub fn with_blocks(def: Block, mut table: Vec) -> Self { + table.push(def); Self { table } } + /// Fetch the default block for this jump table. + pub fn default_block(&self) -> Block { + *self.table.last().unwrap() + } + /// Get the number of table entries. pub fn len(&self) -> usize { - self.table.len() + self.table.len() - 1 } /// Append a table entry. pub fn push_entry(&mut self, dest: Block) { - self.table.push(dest) + let last = self.table.len(); + self.table.push(dest); + + // Ensure that the default block stays as the final element in the table. + self.table.swap(last - 1, last); } - /// Checks if any of the entries branch to `block`. + /// Checks if any of the entries branch to `block`. The default block will be considered when + /// checking for a possible branch. pub fn branches_to(&self, block: Block) -> bool { self.table.iter().any(|target_block| *target_block == block) } - /// Access the whole table as a slice. + /// Access the jump table as a slice, excluding the default block. + pub fn table_slice(&self) -> &[Block] { + let last = self.len(); + &self.table.as_slice()[0..last] + } + + /// Access the jump table as a slice, excluding the default block. + pub fn table_slice_mut(&mut self) -> &mut [Block] { + let last = self.len(); + &mut self.table.as_mut_slice()[0..last] + } + + /// Access the entire jump table as a slice. pub fn as_slice(&self) -> &[Block] { self.table.as_slice() } - /// Access the whole table as a mutable slice. + /// Access the whole table as a mutable slice, excluding the default block. pub fn as_mut_slice(&mut self) -> &mut [Block] { self.table.as_mut_slice() } @@ -68,26 +89,25 @@ impl JumpTableData { self.table.iter() } - /// Returns an iterator that allows modifying each value. + /// Returns an iterator that allows modifying each value, including the default block. pub fn iter_mut(&mut self) -> IterMut { self.table.iter_mut() } - /// Clears all entries in this jump table. + /// Clears all entries in this jump table, except for the default block. pub fn clear(&mut self) { - self.table.clear(); + self.table.drain(0..self.table.len() - 1); } } impl Display for JumpTableData { fn fmt(&self, fmt: &mut Formatter) -> fmt::Result { - write!(fmt, "[")?; - match self.table.first() { - None => (), - Some(first) => write!(fmt, "{}", first)?, - } - for block in self.table.iter().skip(1) { - write!(fmt, ", {}", block)?; + write!(fmt, "{}, [", self.default_block())?; + if let Some((first, rest)) = self.table_slice().split_first() { + write!(fmt, "{}", first)?; + for block in rest { + write!(fmt, ", {}", block)?; + } } write!(fmt, "]") } @@ -102,31 +122,41 @@ mod tests { #[test] fn empty() { - let jt = JumpTableData::new(); + let def = Block::new(0); + + let jt = JumpTableData::new(def); - assert_eq!(jt.as_slice().get(0), None); + assert_eq!(jt.table_slice().get(0), None); + + assert_eq!(jt.as_slice().get(0), Some(&def)); assert_eq!(jt.as_slice().get(10), None); - assert_eq!(jt.to_string(), "[]"); + assert_eq!(jt.to_string(), "block0, []"); + + assert_eq!(jt.as_slice(), [def]); - let v = jt.as_slice(); - assert_eq!(v, []); + assert_eq!(jt.table_slice(), []); } #[test] fn insert() { + let def = Block::new(0); let e1 = Block::new(1); let e2 = Block::new(2); - let mut jt = JumpTableData::new(); + let mut jt = JumpTableData::new(def); jt.push_entry(e1); jt.push_entry(e2); jt.push_entry(e1); - assert_eq!(jt.to_string(), "[block1, block2, block1]"); + assert_eq!(jt.default_block(), def); + assert_eq!(jt.to_string(), "block0, [block1, block2, block1]"); + + assert_eq!(jt.as_slice(), [e1, e2, e1, def]); + assert_eq!(jt.table_slice(), [e1, e2, e1]); - let v = jt.as_slice(); - assert_eq!(v, [e1, e2, e1]); + jt.clear(); + assert_eq!(jt.default_block(), def); } } diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index eccda08b60b9..c42bc2c41161 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -2515,7 +2515,7 @@ ;; `targets` contains the default target with the list of branch targets ;; concatenated. -(rule (lower_branch (br_table idx _ _) targets) +(rule (lower_branch (br_table idx _) targets) (let ((jt_size u32 (targets_jt_size targets)) (_ InstOutput (side_effect (emit_island (targets_jt_space targets)))) diff --git a/cranelift/codegen/src/isa/aarch64/mod.rs b/cranelift/codegen/src/isa/aarch64/mod.rs index d7a2630ae846..671a8fb7c8dc 100644 --- a/cranelift/codegen/src/isa/aarch64/mod.rs +++ b/cranelift/codegen/src/isa/aarch64/mod.rs @@ -361,11 +361,11 @@ mod test { let mut pos = FuncCursor::new(&mut func); pos.insert_block(bb0); - let mut jt_data = JumpTableData::new(); + let mut jt_data = JumpTableData::new(bb3); jt_data.push_entry(bb1); jt_data.push_entry(bb2); let jt = pos.func.create_jump_table(jt_data); - pos.ins().br_table(arg0, bb3, jt); + pos.ins().br_table(arg0, jt); pos.insert_block(bb1); let v1 = pos.ins().iconst(I32, 1); diff --git a/cranelift/codegen/src/isa/riscv64/inst.isle b/cranelift/codegen/src/isa/riscv64/inst.isle index ef6f8dd3793f..10bcbd193bfd 100644 --- a/cranelift/codegen/src/isa/riscv64/inst.isle +++ b/cranelift/codegen/src/isa/riscv64/inst.isle @@ -1942,7 +1942,7 @@ (extern constructor lower_br_table lower_br_table) (rule - (lower_branch (br_table index _ _) targets) + (lower_branch (br_table index _) targets) (lower_br_table index targets)) (decl load_ra () Reg) diff --git a/cranelift/codegen/src/isa/s390x/lower.isle b/cranelift/codegen/src/isa/s390x/lower.isle index 1eed87da070c..f1768f95d51b 100644 --- a/cranelift/codegen/src/isa/s390x/lower.isle +++ b/cranelift/codegen/src/isa/s390x/lower.isle @@ -3725,7 +3725,7 @@ ;; Jump table. `targets` contains the default target followed by the ;; list of branch targets per index value. -(rule (lower_branch (br_table val_idx _ _) targets) +(rule (lower_branch (br_table val_idx _) targets) (let ((idx Reg (put_in_reg_zext64 val_idx)) ;; Bounds-check the index and branch to default. ;; This is an internal branch that is not a terminator insn. diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index 9adc4e2fdb55..e8fd01f840a8 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -2942,7 +2942,7 @@ ;; Rules for `br_table` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -(rule (lower_branch (br_table idx @ (value_type ty) _ _) (jump_table_targets default_target jt_targets)) +(rule (lower_branch (br_table idx @ (value_type ty) _) (jump_table_targets default_target jt_targets)) (emit_side_effect (jmp_table_seq ty idx default_target jt_targets))) ;; Rules for `select_spectre_guard` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/cranelift/codegen/src/isa/x64/mod.rs b/cranelift/codegen/src/isa/x64/mod.rs index a567ddf21448..2482536e2873 100644 --- a/cranelift/codegen/src/isa/x64/mod.rs +++ b/cranelift/codegen/src/isa/x64/mod.rs @@ -408,11 +408,11 @@ mod test { let mut pos = FuncCursor::new(&mut func); pos.insert_block(bb0); - let mut jt_data = JumpTableData::new(); + let mut jt_data = JumpTableData::new(bb3); jt_data.push_entry(bb1); jt_data.push_entry(bb2); let jt = pos.func.create_jump_table(jt_data); - pos.ins().br_table(arg0, bb3, jt); + pos.ins().br_table(arg0, jt); pos.insert_block(bb1); let v1 = pos.ins().iconst(I32, 1); diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index e9fff15ff424..be1cabb15870 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -579,10 +579,7 @@ impl<'a> Verifier<'a> { self.verify_block(inst, block_then.block(&self.func.dfg.value_lists), errors)?; self.verify_block(inst, block_else.block(&self.func.dfg.value_lists), errors)?; } - BranchTable { - table, destination, .. - } => { - self.verify_block(inst, destination, errors)?; + BranchTable { table, .. } => { self.verify_jump_table(inst, table, errors)?; } Call { @@ -852,7 +849,7 @@ impl<'a> Verifier<'a> { format!("invalid jump table reference {}", j), )) } else { - for &block in self.func.stencil.dfg.jump_tables[j].as_slice() { + for &block in self.func.stencil.dfg.jump_tables[j].iter() { self.verify_block(inst, block, errors)?; } Ok(()) @@ -1322,22 +1319,7 @@ impl<'a> Verifier<'a> { let args_else = block_else.args_slice(&self.func.dfg.value_lists); self.typecheck_variable_args_iterator(inst, iter, args_else, errors)?; } - ir::InstructionData::BranchTable { - table, - destination: block, - .. - } => { - let arg_count = self.func.dfg.num_block_params(*block); - if arg_count != 0 { - return errors.nonfatal(( - inst, - self.context(inst), - format!( - "takes no arguments, but had target {} with {} arguments", - block, arg_count, - ), - )); - } + ir::InstructionData::BranchTable { table, .. } => { for block in self.func.stencil.dfg.jump_tables[*table].iter() { let arg_count = self.func.dfg.num_block_params(*block); if arg_count != 0 { diff --git a/cranelift/codegen/src/write.rs b/cranelift/codegen/src/write.rs index 0b4c1d9da42f..3f77fff50d7c 100644 --- a/cranelift/codegen/src/write.rs +++ b/cranelift/codegen/src/write.rs @@ -421,12 +421,7 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt write!(w, " {}, {}", arg, block_then.display(pool))?; write!(w, ", {}", block_else.display(pool)) } - BranchTable { - arg, - destination, - table, - .. - } => write!(w, " {}, {}, {}", arg, destination, jump_tables[table]), + BranchTable { arg, table, .. } => write!(w, " {}, {}", arg, jump_tables[table]), Call { func_ref, ref args, .. } => write!(w, " {}({})", func_ref, DisplayValues(args.as_slice(pool))), diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index db9875602b37..69d9b06eace4 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -129,9 +129,7 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { } } - ir::InstructionData::BranchTable { - table, destination, .. - } => { + ir::InstructionData::BranchTable { table, .. } => { // Unlike all other jumps/branches, jump tables are // capable of having the same successor appear // multiple times, so we must deduplicate. @@ -154,7 +152,6 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { .ssa .declare_block_predecessor(*dest_block, inst); } - self.builder.declare_successor(*destination, inst); } inst => debug_assert!(!inst.opcode().is_branch()), diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index d6c852e2f6ec..f23609e55596 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -591,11 +591,7 @@ impl SSABuilder { } None } - InstructionData::BranchTable { - table: jt, - destination, - .. - } => { + InstructionData::BranchTable { table: jt, .. } => { // In the case of a jump table, the situation is tricky because br_table doesn't // support arguments. We have to split the critical edge. let middle_block = dfg.blocks.add(); @@ -608,10 +604,6 @@ impl SSABuilder { } } - if *destination == dest_block { - *destination = middle_block; - } - let mut cur = FuncCursor::new(func).at_bottom(middle_block); let middle_jump_inst = cur.ins().jump(dest_block, &[val]); Some((middle_block, middle_jump_inst)) @@ -1001,7 +993,7 @@ mod tests { let block0 = func.dfg.make_block(); let block1 = func.dfg.make_block(); let block2 = func.dfg.make_block(); - let mut jump_table = JumpTableData::new(); + let mut jump_table = JumpTableData::new(block2); jump_table.push_entry(block2); jump_table.push_entry(block1); { @@ -1024,7 +1016,7 @@ mod tests { let br_table = { let jt = func.create_jump_table(jump_table); let mut cur = FuncCursor::new(&mut func).at_bottom(block0); - cur.ins().br_table(x1, block2, jt) + cur.ins().br_table(x1, jt) }; // block1 diff --git a/cranelift/frontend/src/switch.rs b/cranelift/frontend/src/switch.rs index f7ff7f87436f..121bc7cb7dc7 100644 --- a/cranelift/frontend/src/switch.rs +++ b/cranelift/frontend/src/switch.rs @@ -220,7 +220,7 @@ impl Switch { "Jump tables bigger than 2^32-1 are not yet supported" ); - let jt_data = JumpTableData::with_blocks(Vec::from(blocks)); + let jt_data = JumpTableData::with_blocks(otherwise, Vec::from(blocks)); let jump_table = bx.create_jump_table(jt_data); let discr = if first_index == 0 { @@ -256,7 +256,7 @@ impl Switch { _ => discr, }; - bx.ins().br_table(discr, otherwise, jump_table); + bx.ins().br_table(discr, jump_table); } /// Build the switch diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 4ffc3f679445..624dfaccfb99 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -1614,12 +1614,12 @@ where } BlockTerminator::BrTable(default, targets) => { // Create jump tables on demand - let jt = builder.create_jump_table(JumpTableData::with_blocks(targets)); + let jt = builder.create_jump_table(JumpTableData::with_blocks(default, targets)); // br_table only supports I32 let val = builder.use_var(self.get_variable_of_type(I32)?); - builder.ins().br_table(val, default, jt); + builder.ins().br_table(val, jt); } BlockTerminator::Switch(_type, default, entries) => { let mut switch = Switch::new(); diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs index 7973b846e632..64508756e882 100644 --- a/cranelift/interpreter/src/step.rs +++ b/cranelift/interpreter/src/step.rs @@ -373,18 +373,15 @@ where } } Opcode::BrTable => { - if let InstructionData::BranchTable { - table, destination, .. - } = inst - { + if let InstructionData::BranchTable { table, .. } = inst { let jt_data = &state.get_current_function().stencil.dfg.jump_tables[table]; // Convert to usize to remove negative indexes from the following operations let jump_target = usize::try_from(arg(0)?.into_int()?) .ok() - .and_then(|i| jt_data.as_slice().get(i)) + .and_then(|i| jt_data.table_slice().get(i)) .copied() - .unwrap_or(destination); + .unwrap_or(jt_data.default_block()); ControlFlow::ContinueAt(jump_target, SmallVec::new()) } else { diff --git a/cranelift/reader/src/parser.rs b/cranelift/reader/src/parser.rs index 2154219bb474..8fd443f690ab 100644 --- a/cranelift/reader/src/parser.rs +++ b/cranelift/reader/src/parser.rs @@ -1767,10 +1767,10 @@ impl<'a> Parser<'a> { // // jump-table-lit ::= "[" block {"," block } "]" // | "[]" - fn parse_jump_table(&mut self) -> ParseResult { + fn parse_jump_table(&mut self, def: Block) -> ParseResult { self.match_token(Token::LBracket, "expected '[' before jump table contents")?; - let mut data = JumpTableData::new(); + let mut data = JumpTableData::new(def); match self.token() { Some(Token::Block(dest)) => { @@ -2571,14 +2571,9 @@ impl<'a> Parser<'a> { self.match_token(Token::Comma, "expected ',' between operands")?; let block_num = self.match_block("expected branch destination block")?; self.match_token(Token::Comma, "expected ',' between operands")?; - let table_data = self.parse_jump_table()?; + let table_data = self.parse_jump_table(block_num)?; let table = ctx.function.dfg.jump_tables.push(table_data); - InstructionData::BranchTable { - opcode, - arg, - destination: block_num, - table, - } + InstructionData::BranchTable { opcode, arg, table } } InstructionFormat::TernaryImm8 => { let lhs = self.match_value("expected SSA value first operand")?; diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 22032a0f29e0..77a1ec21b35f 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -515,7 +515,7 @@ pub fn translate_operator( } }; let val = state.pop1(); - let mut data = JumpTableData::with_capacity(targets.len() as usize); + let mut data = Vec::with_capacity(targets.len() as usize); if jump_args_count == 0 { // No jump arguments for depth in targets.targets() { @@ -526,16 +526,16 @@ pub fn translate_operator( frame.set_branched_to_exit(); frame.br_destination() }; - data.push_entry(block); + data.push(block); } - let jt = builder.create_jump_table(data); let block = { let i = state.control_stack.len() - 1 - (default as usize); let frame = &mut state.control_stack[i]; frame.set_branched_to_exit(); frame.br_destination() }; - builder.ins().br_table(val, block, jt); + let jt = builder.create_jump_table(JumpTableData::with_blocks(block, data)); + builder.ins().br_table(val, jt); } else { // Here we have jump arguments, but Cranelift's br_table doesn't support them // We then proceed to split the edges going out of the br_table @@ -552,7 +552,7 @@ pub fn translate_operator( *entry.insert(block) } }; - data.push_entry(branch_block); + data.push(branch_block); } let default_branch_block = match dest_block_map.entry(default as usize) { hash_map::Entry::Occupied(entry) => *entry.get(), @@ -562,8 +562,9 @@ pub fn translate_operator( *entry.insert(block) } }; - let jt = builder.create_jump_table(data); - builder.ins().br_table(val, default_branch_block, jt); + let jt = builder + .create_jump_table(JumpTableData::with_blocks(default_branch_block, data)); + builder.ins().br_table(val, jt); for (depth, dest_block) in dest_block_sequence { builder.switch_to_block(dest_block); builder.seal_block(dest_block); From bb4cd8ce5ac777517e801e05020103ff989d07f6 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 15:55:09 -0800 Subject: [PATCH 2/7] Preserve the old behavior of JumpTableData functions --- cranelift/codegen/src/inst_predicates.rs | 2 +- cranelift/codegen/src/ir/function.rs | 4 +- cranelift/codegen/src/ir/jumptable.rs | 89 +++++++++--------------- cranelift/codegen/src/verifier/mod.rs | 4 +- cranelift/frontend/src/frontend.rs | 7 +- cranelift/frontend/src/ssa.rs | 7 +- cranelift/frontend/src/switch.rs | 2 +- cranelift/interpreter/src/step.rs | 2 +- cranelift/reader/src/parser.rs | 8 +-- cranelift/wasm/src/code_translator.rs | 5 +- 10 files changed, 52 insertions(+), 78 deletions(-) diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs index 2e876e1045fe..450a09e3f81e 100644 --- a/cranelift/codegen/src/inst_predicates.rs +++ b/cranelift/codegen/src/inst_predicates.rs @@ -197,7 +197,7 @@ pub(crate) fn visit_block_succs( // so it is not part of the table. visit(inst, table.default_block(), false); - for &dest in table.table_slice() { + for &dest in table.as_slice() { visit(inst, dest, true); } } diff --git a/cranelift/codegen/src/ir/function.rs b/cranelift/codegen/src/ir/function.rs index 8a2c3e66cfd1..3e0a00b17719 100644 --- a/cranelift/codegen/src/ir/function.rs +++ b/cranelift/codegen/src/ir/function.rs @@ -296,11 +296,11 @@ impl FunctionStencil { } InstructionData::BranchTable { table, .. } => { - self.dfg.jump_tables[*table].iter_mut().for_each(|entry| { + for entry in self.dfg.jump_tables[*table].all_branches_mut() { if *entry == old_dest { *entry = new_dest; } - }); + } } inst => debug_assert!(!inst.opcode().is_branch()), diff --git a/cranelift/codegen/src/ir/jumptable.rs b/cranelift/codegen/src/ir/jumptable.rs index 427bba05168d..8b401ab4a816 100644 --- a/cranelift/codegen/src/ir/jumptable.rs +++ b/cranelift/codegen/src/ir/jumptable.rs @@ -26,13 +26,8 @@ pub struct JumpTableData { } impl JumpTableData { - /// Create a new empty jump table. - pub fn new(def: Block) -> Self { - Self { table: vec![def] } - } - /// Create a new jump table with the provided blocks - pub fn with_blocks(def: Block, mut table: Vec) -> Self { + pub fn new(def: Block, mut table: Vec) -> Self { table.push(def); Self { table } } @@ -42,56 +37,44 @@ impl JumpTableData { *self.table.last().unwrap() } - /// Get the number of table entries. - pub fn len(&self) -> usize { - self.table.len() - 1 - } - - /// Append a table entry. - pub fn push_entry(&mut self, dest: Block) { - let last = self.table.len(); - self.table.push(dest); - - // Ensure that the default block stays as the final element in the table. - self.table.swap(last - 1, last); - } - - /// Checks if any of the entries branch to `block`. The default block will be considered when - /// checking for a possible branch. - pub fn branches_to(&self, block: Block) -> bool { - self.table.iter().any(|target_block| *target_block == block) + /// Mutable access to the default block of this jump table. + pub fn default_block_mut(&mut self) -> &mut Block { + self.table.last_mut().unwrap() } - /// Access the jump table as a slice, excluding the default block. - pub fn table_slice(&self) -> &[Block] { - let last = self.len(); - &self.table.as_slice()[0..last] + /// The jump table and default block as a single slice. The default block will always be last. + pub fn all_branches(&self) -> &[Block] { + self.table.as_slice() } - /// Access the jump table as a slice, excluding the default block. - pub fn table_slice_mut(&mut self) -> &mut [Block] { - let last = self.len(); - &mut self.table.as_mut_slice()[0..last] + /// The jump table and default block as a single mutable slice. The default block will always + /// be last. + pub fn all_branches_mut(&mut self) -> &mut [Block] { + self.table.as_mut_slice() } - /// Access the entire jump table as a slice. + /// Access the jump table as a slice. This excludes the default block. pub fn as_slice(&self) -> &[Block] { - self.table.as_slice() + let last = self.table.len() - 1; + &self.table.as_slice()[0..last] } - /// Access the whole table as a mutable slice, excluding the default block. + /// Access the jump table as a mutable slice. This excludes the default block. pub fn as_mut_slice(&mut self) -> &mut [Block] { - self.table.as_mut_slice() + let last = self.table.len() - 1; + &mut self.table.as_mut_slice()[0..last] } - /// Returns an iterator over the table. + /// Returns an iterator to the jump table, excluding the default block. + #[deprecated(since = "0.7.0", note = "please use `.as_slice()` instead")] pub fn iter(&self) -> Iter { - self.table.iter() + self.as_slice().iter() } - /// Returns an iterator that allows modifying each value, including the default block. + /// Returns an iterator that allows modifying each value, excluding the default block. + #[deprecated(since = "0.7.0", note = "please use `.as_mut_slice()` instead")] pub fn iter_mut(&mut self) -> IterMut { - self.table.iter_mut() + self.as_mut_slice().iter_mut() } /// Clears all entries in this jump table, except for the default block. @@ -103,7 +86,7 @@ impl JumpTableData { impl Display for JumpTableData { fn fmt(&self, fmt: &mut Formatter) -> fmt::Result { write!(fmt, "{}, [", self.default_block())?; - if let Some((first, rest)) = self.table_slice().split_first() { + if let Some((first, rest)) = self.as_slice().split_first() { write!(fmt, "{}", first)?; for block in rest { write!(fmt, ", {}", block)?; @@ -124,18 +107,17 @@ mod tests { fn empty() { let def = Block::new(0); - let jt = JumpTableData::new(def); + let jt = JumpTableData::new(def, vec![]); - assert_eq!(jt.table_slice().get(0), None); + assert_eq!(jt.all_branches().get(0), Some(&def)); - assert_eq!(jt.as_slice().get(0), Some(&def)); + assert_eq!(jt.as_slice().get(0), None); assert_eq!(jt.as_slice().get(10), None); assert_eq!(jt.to_string(), "block0, []"); - assert_eq!(jt.as_slice(), [def]); - - assert_eq!(jt.table_slice(), []); + assert_eq!(jt.all_branches(), [def]); + assert_eq!(jt.as_slice(), []); } #[test] @@ -144,19 +126,12 @@ mod tests { let e1 = Block::new(1); let e2 = Block::new(2); - let mut jt = JumpTableData::new(def); - - jt.push_entry(e1); - jt.push_entry(e2); - jt.push_entry(e1); + let mut jt = JumpTableData::new(def, vec![e1, e2, e1]); assert_eq!(jt.default_block(), def); assert_eq!(jt.to_string(), "block0, [block1, block2, block1]"); - assert_eq!(jt.as_slice(), [e1, e2, e1, def]); - assert_eq!(jt.table_slice(), [e1, e2, e1]); - - jt.clear(); - assert_eq!(jt.default_block(), def); + assert_eq!(jt.all_branches(), [e1, e2, e1, def]); + assert_eq!(jt.as_slice(), [e1, e2, e1]); } } diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index be1cabb15870..4572a5a2ecaa 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -849,7 +849,7 @@ impl<'a> Verifier<'a> { format!("invalid jump table reference {}", j), )) } else { - for &block in self.func.stencil.dfg.jump_tables[j].iter() { + for &block in self.func.stencil.dfg.jump_tables[j].all_branches() { self.verify_block(inst, block, errors)?; } Ok(()) @@ -1320,7 +1320,7 @@ impl<'a> Verifier<'a> { self.typecheck_variable_args_iterator(inst, iter, args_else, errors)?; } ir::InstructionData::BranchTable { table, .. } => { - for block in self.func.stencil.dfg.jump_tables[*table].iter() { + for block in self.func.stencil.dfg.jump_tables[*table].all_branches() { let arg_count = self.func.dfg.num_block_params(*block); if arg_count != 0 { return errors.nonfatal(( diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index 69d9b06eace4..6f405757f672 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -142,9 +142,12 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { .jump_tables .get(*table) .expect("you are referencing an undeclared jump table") - .iter() - .filter(|&dest_block| unique.insert(*dest_block)) + .all_branches() { + if !unique.insert(*dest_block) { + continue; + } + // Call `declare_block_predecessor` instead of `declare_successor` for // avoiding the borrow checker. self.builder diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index f23609e55596..c388ee425aaa 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -597,8 +597,7 @@ impl SSABuilder { let middle_block = dfg.blocks.add(); func.stencil.layout.append_block(middle_block); - let table = &mut dfg.jump_tables[*jt]; - for block in table.iter_mut() { + for block in dfg.jump_tables[*jt].all_branches_mut() { if *block == dest_block { *block = middle_block; } @@ -993,9 +992,6 @@ mod tests { let block0 = func.dfg.make_block(); let block1 = func.dfg.make_block(); let block2 = func.dfg.make_block(); - let mut jump_table = JumpTableData::new(block2); - jump_table.push_entry(block2); - jump_table.push_entry(block1); { let mut cur = FuncCursor::new(&mut func); cur.insert_block(block0); @@ -1014,6 +1010,7 @@ mod tests { ssa.def_var(x_var, x1, block0); ssa.use_var(&mut func, x_var, I32, block0).0; let br_table = { + let jump_table = JumpTableData::new(block2, vec![block2, block1]); let jt = func.create_jump_table(jump_table); let mut cur = FuncCursor::new(&mut func).at_bottom(block0); cur.ins().br_table(x1, jt) diff --git a/cranelift/frontend/src/switch.rs b/cranelift/frontend/src/switch.rs index 121bc7cb7dc7..7788283286d2 100644 --- a/cranelift/frontend/src/switch.rs +++ b/cranelift/frontend/src/switch.rs @@ -220,7 +220,7 @@ impl Switch { "Jump tables bigger than 2^32-1 are not yet supported" ); - let jt_data = JumpTableData::with_blocks(otherwise, Vec::from(blocks)); + let jt_data = JumpTableData::new(otherwise, Vec::from(blocks)); let jump_table = bx.create_jump_table(jt_data); let discr = if first_index == 0 { diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs index 64508756e882..98b699b1caa2 100644 --- a/cranelift/interpreter/src/step.rs +++ b/cranelift/interpreter/src/step.rs @@ -379,7 +379,7 @@ where // Convert to usize to remove negative indexes from the following operations let jump_target = usize::try_from(arg(0)?.into_int()?) .ok() - .and_then(|i| jt_data.table_slice().get(i)) + .and_then(|i| jt_data.as_slice().get(i)) .copied() .unwrap_or(jt_data.default_block()); diff --git a/cranelift/reader/src/parser.rs b/cranelift/reader/src/parser.rs index 8fd443f690ab..b0cb2baac5e0 100644 --- a/cranelift/reader/src/parser.rs +++ b/cranelift/reader/src/parser.rs @@ -1770,12 +1770,12 @@ impl<'a> Parser<'a> { fn parse_jump_table(&mut self, def: Block) -> ParseResult { self.match_token(Token::LBracket, "expected '[' before jump table contents")?; - let mut data = JumpTableData::new(def); + let mut data = Vec::new(); match self.token() { Some(Token::Block(dest)) => { self.consume(); - data.push_entry(dest); + data.push(dest); loop { match self.token() { @@ -1783,7 +1783,7 @@ impl<'a> Parser<'a> { self.consume(); if let Some(Token::Block(dest)) = self.token() { self.consume(); - data.push_entry(dest); + data.push(dest); } else { return err!(self.loc, "expected jump_table entry"); } @@ -1799,7 +1799,7 @@ impl<'a> Parser<'a> { self.consume(); - Ok(data) + Ok(JumpTableData::new(def, data)) } // Parse a constant decl. diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 77a1ec21b35f..1d7fd2d9590b 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -534,7 +534,7 @@ pub fn translate_operator( frame.set_branched_to_exit(); frame.br_destination() }; - let jt = builder.create_jump_table(JumpTableData::with_blocks(block, data)); + let jt = builder.create_jump_table(JumpTableData::new(block, data)); builder.ins().br_table(val, jt); } else { // Here we have jump arguments, but Cranelift's br_table doesn't support them @@ -562,8 +562,7 @@ pub fn translate_operator( *entry.insert(block) } }; - let jt = builder - .create_jump_table(JumpTableData::with_blocks(default_branch_block, data)); + let jt = builder.create_jump_table(JumpTableData::new(default_branch_block, data)); builder.ins().br_table(val, jt); for (depth, dest_block) in dest_block_sequence { builder.switch_to_block(dest_block); From 25127f1448ebc008c325be140823442caa1deb8f Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 16:01:07 -0800 Subject: [PATCH 3/7] Rework some uses of `push_entry` --- cranelift/codegen/src/isa/aarch64/mod.rs | 7 +++---- cranelift/codegen/src/isa/x64/mod.rs | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cranelift/codegen/src/isa/aarch64/mod.rs b/cranelift/codegen/src/isa/aarch64/mod.rs index 671a8fb7c8dc..83454910283d 100644 --- a/cranelift/codegen/src/isa/aarch64/mod.rs +++ b/cranelift/codegen/src/isa/aarch64/mod.rs @@ -361,10 +361,9 @@ mod test { let mut pos = FuncCursor::new(&mut func); pos.insert_block(bb0); - let mut jt_data = JumpTableData::new(bb3); - jt_data.push_entry(bb1); - jt_data.push_entry(bb2); - let jt = pos.func.create_jump_table(jt_data); + let jt = pos + .func + .create_jump_table(JumpTableData::new(bb3, vec![bb1, bb2])); pos.ins().br_table(arg0, jt); pos.insert_block(bb1); diff --git a/cranelift/codegen/src/isa/x64/mod.rs b/cranelift/codegen/src/isa/x64/mod.rs index 2482536e2873..a7bda0a46642 100644 --- a/cranelift/codegen/src/isa/x64/mod.rs +++ b/cranelift/codegen/src/isa/x64/mod.rs @@ -408,10 +408,9 @@ mod test { let mut pos = FuncCursor::new(&mut func); pos.insert_block(bb0); - let mut jt_data = JumpTableData::new(bb3); - jt_data.push_entry(bb1); - jt_data.push_entry(bb2); - let jt = pos.func.create_jump_table(jt_data); + let jt = pos + .func + .create_jump_table(JumpTableData::new(bb3, vec![bb1, bb2])); pos.ins().br_table(arg0, jt); pos.insert_block(bb1); From f8c2a46b420011b6ec20b01ff036a11c83d4fa23 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 16:08:17 -0800 Subject: [PATCH 4/7] Fix deprecation version --- cranelift/codegen/src/ir/jumptable.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cranelift/codegen/src/ir/jumptable.rs b/cranelift/codegen/src/ir/jumptable.rs index 8b401ab4a816..dbe9581a8ea4 100644 --- a/cranelift/codegen/src/ir/jumptable.rs +++ b/cranelift/codegen/src/ir/jumptable.rs @@ -66,13 +66,13 @@ impl JumpTableData { } /// Returns an iterator to the jump table, excluding the default block. - #[deprecated(since = "0.7.0", note = "please use `.as_slice()` instead")] + #[deprecated(since = "7.0.0", note = "please use `.as_slice()` instead")] pub fn iter(&self) -> Iter { self.as_slice().iter() } /// Returns an iterator that allows modifying each value, excluding the default block. - #[deprecated(since = "0.7.0", note = "please use `.as_mut_slice()` instead")] + #[deprecated(since = "7.0.0", note = "please use `.as_mut_slice()` instead")] pub fn iter_mut(&mut self) -> IterMut { self.as_mut_slice().iter_mut() } From dd61d048e43dfa7d802d57572c4c1ba85449dd47 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 16:55:14 -0800 Subject: [PATCH 5/7] Fix a warning --- cranelift/codegen/src/ir/jumptable.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cranelift/codegen/src/ir/jumptable.rs b/cranelift/codegen/src/ir/jumptable.rs index dbe9581a8ea4..cd8d20d0dc8a 100644 --- a/cranelift/codegen/src/ir/jumptable.rs +++ b/cranelift/codegen/src/ir/jumptable.rs @@ -126,7 +126,7 @@ mod tests { let e1 = Block::new(1); let e2 = Block::new(2); - let mut jt = JumpTableData::new(def, vec![e1, e2, e1]); + let jt = JumpTableData::new(def, vec![e1, e2, e1]); assert_eq!(jt.default_block(), def); assert_eq!(jt.to_string(), "block0, [block1, block2, block1]"); From 71ce26c5c57070376f29dbeb97d5a9b0706801a7 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 16:56:26 -0800 Subject: [PATCH 6/7] Fix a missed function renaming --- cranelift/fuzzgen/src/function_generator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 624dfaccfb99..1c4b68b1874e 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -1614,7 +1614,7 @@ where } BlockTerminator::BrTable(default, targets) => { // Create jump tables on demand - let jt = builder.create_jump_table(JumpTableData::with_blocks(default, targets)); + let jt = builder.create_jump_table(JumpTableData::new(default, targets)); // br_table only supports I32 let val = builder.use_var(self.get_variable_of_type(I32)?); From 8a348772079c3e442af2493da4e2835c20966aa8 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Feb 2023 18:30:01 -0800 Subject: [PATCH 7/7] Comments from code review --- cranelift/codegen/src/inst_predicates.rs | 4 +++- cranelift/codegen/src/ir/jumptable.rs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs index 450a09e3f81e..e6bc6e666452 100644 --- a/cranelift/codegen/src/inst_predicates.rs +++ b/cranelift/codegen/src/inst_predicates.rs @@ -194,7 +194,9 @@ pub(crate) fn visit_block_succs( let table = &f.stencil.dfg.jump_tables[*table]; // The default block is reached via a direct conditional branch, - // so it is not part of the table. + // so it is not part of the table. We visit the default block first + // explicitly, as some callers of visit_block_succs depend on that + // ordering. visit(inst, table.default_block(), false); for &dest in table.as_slice() { diff --git a/cranelift/codegen/src/ir/jumptable.rs b/cranelift/codegen/src/ir/jumptable.rs index cd8d20d0dc8a..70d0b1ddde03 100644 --- a/cranelift/codegen/src/ir/jumptable.rs +++ b/cranelift/codegen/src/ir/jumptable.rs @@ -17,7 +17,9 @@ use serde::{Deserialize, Serialize}; /// /// The default block for the jump table is stored as the last element of the underlying vector, /// and is not included in the length of the jump table. It can be accessed through the -/// `default_block` function. The default block is iterated via the `iter` and `iter_mut` methods. +/// `default_block` and `default_block_mut` functions. All blocks may be iterated using the +/// `all_branches` and `all_branches_mut` functions, which will both iterate over the default block +/// last. #[derive(Clone, PartialEq, Hash)] #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))] pub struct JumpTableData {