Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 18 additions & 20 deletions cranelift/codegen/src/ir/jumptable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ use serde::{Deserialize, Serialize};
///
/// 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` 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.
/// The default block for the jump table is stored as the first element of the underlying vector.
/// It can be accessed through the `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 first.
#[derive(Clone, PartialEq, Hash)]
#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
pub struct JumpTableData {
Expand All @@ -29,42 +28,41 @@ pub struct JumpTableData {

impl JumpTableData {
/// Create a new jump table with the provided blocks
pub fn new(def: Block, mut table: Vec<Block>) -> Self {
table.push(def);
Self { table }
pub fn new(def: Block, table: &[Block]) -> Self {
Self {
table: std::iter::once(def).chain(table.iter().copied()).collect(),
}
Comment on lines +31 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing this I considered two alternative implementations but decided I like the one you've chosen best.

One alternative is to continue to accept a Vec and use table.insert(0, def), which implicitly does a memmove. Some callers already have a Vec allocated and it's a little annoying to have to allocate a second one to copy into, only for the caller to drop theirs immediately afterward.

The other alternative is to take table: impl IntoIterator<Item = Block>. That's somewhat appealing for cranelift/wasm/src/code_translator.rs which I think could then avoid allocating a temporary Vec. It also works fine for all the tests since arrays implement the right IntoIterator. Passing in a Vec by-value works too, although like the current approach that still allocates a new Vec and copies into it. However, I think this is a little more magic than necessary. Passing in a slice is fine.

}

/// Fetch the default block for this jump table.
pub fn default_block(&self) -> Block {
*self.table.last().unwrap()
*self.table.first().unwrap()
}

/// Mutable access to the default block of this jump table.
pub fn default_block_mut(&mut self) -> &mut Block {
self.table.last_mut().unwrap()
self.table.first_mut().unwrap()
}

/// The jump table and default block as a single slice. The default block will always be last.
/// The jump table and default block as a single slice. The default block will always be first.
pub fn all_branches(&self) -> &[Block] {
self.table.as_slice()
}

/// The jump table and default block as a single mutable slice. The default block will always
/// be last.
/// be first.
pub fn all_branches_mut(&mut self) -> &mut [Block] {
self.table.as_mut_slice()
}

/// Access the jump table as a slice. This excludes the default block.
pub fn as_slice(&self) -> &[Block] {
let last = self.table.len() - 1;
&self.table.as_slice()[0..last]
&self.table.as_slice()[1..]
}

/// Access the jump table as a mutable slice. This excludes the default block.
pub fn as_mut_slice(&mut self) -> &mut [Block] {
let last = self.table.len() - 1;
&mut self.table.as_mut_slice()[0..last]
&mut self.table.as_mut_slice()[1..]
}

/// Returns an iterator to the jump table, excluding the default block.
Expand All @@ -81,7 +79,7 @@ impl JumpTableData {

/// Clears all entries in this jump table, except for the default block.
pub fn clear(&mut self) {
self.table.drain(0..self.table.len() - 1);
self.table.drain(1..);
}
}

Expand Down Expand Up @@ -109,7 +107,7 @@ mod tests {
fn empty() {
let def = Block::new(0);

let jt = JumpTableData::new(def, vec![]);
let jt = JumpTableData::new(def, &[]);

assert_eq!(jt.all_branches().get(0), Some(&def));

Expand All @@ -128,12 +126,12 @@ mod tests {
let e1 = Block::new(1);
let e2 = Block::new(2);

let jt = JumpTableData::new(def, vec![e1, e2, e1]);
let jt = JumpTableData::new(def, &[e1, e2, e1]);

assert_eq!(jt.default_block(), def);
assert_eq!(jt.to_string(), "block0, [block1, block2, block1]");

assert_eq!(jt.all_branches(), [e1, e2, e1, def]);
assert_eq!(jt.all_branches(), [def, e1, e2, e1]);
assert_eq!(jt.as_slice(), [e1, e2, e1]);
}
}
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/aarch64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ mod test {
pos.insert_block(bb0);
let jt = pos
.func
.create_jump_table(JumpTableData::new(bb3, vec![bb1, bb2]));
.create_jump_table(JumpTableData::new(bb3, &[bb1, bb2]));
pos.ins().br_table(arg0, jt);

pos.insert_block(bb1);
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/x64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ mod test {
pos.insert_block(bb0);
let jt = pos
.func
.create_jump_table(JumpTableData::new(bb3, vec![bb1, bb2]));
.create_jump_table(JumpTableData::new(bb3, &[bb1, bb2]));
pos.ins().br_table(arg0, jt);

pos.insert_block(bb1);
Expand Down
2 changes: 1 addition & 1 deletion cranelift/frontend/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1010,7 +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 jump_table = JumpTableData::new(block2, &[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)
Expand Down
2 changes: 1 addition & 1 deletion cranelift/frontend/src/switch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ impl Switch {
"Jump tables bigger than 2^32-1 are not yet supported"
);

let jt_data = JumpTableData::new(otherwise, Vec::from(blocks));
let jt_data = JumpTableData::new(otherwise, blocks);
let jump_table = bx.create_jump_table(jt_data);

let discr = if first_index == 0 {
Expand Down
2 changes: 1 addition & 1 deletion cranelift/fuzzgen/src/function_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1614,7 +1614,7 @@ where
}
BlockTerminator::BrTable(default, targets) => {
// Create jump tables on demand
let jt = builder.create_jump_table(JumpTableData::new(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)?);
Expand Down
2 changes: 1 addition & 1 deletion cranelift/reader/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1799,7 +1799,7 @@ impl<'a> Parser<'a> {

self.consume();

Ok(JumpTableData::new(def, data))
Ok(JumpTableData::new(def, &data))
}

// Parse a constant decl.
Expand Down
4 changes: 2 additions & 2 deletions cranelift/wasm/src/code_translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
frame.set_branched_to_exit();
frame.br_destination()
};
let jt = builder.create_jump_table(JumpTableData::new(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
Expand Down Expand Up @@ -562,7 +562,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
*entry.insert(block)
}
};
let jt = builder.create_jump_table(JumpTableData::new(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);
Expand Down