Skip to content

Commit

Permalink
wasmtime: Implement table.get and table.set
Browse files Browse the repository at this point in the history
These instructions have fast, inline JIT paths for the common cases, and only
call out to host VM functions for the slow paths. This required some changes to
`cranelift-wasm`'s `FuncEnvironment`: instead of taking a `FuncCursor` to insert
an instruction sequence within the current basic block,
`FuncEnvironment::translate_table_{get,set}` now take a `&mut FunctionBuilder`
so that they can create whole new basic blocks. This is necessary for
implementing GC read/write barriers that involve branching (e.g. checking for
null, or whether a store buffer is at capacity).

Furthermore, it required that the `load`, `load_complex`, and `store`
instructions handle loading and storing through an `r{32,64}` rather than just
`i{32,64}` addresses. This involved making `r{32,64}` types acceptable
instantiations of the `iAddr` type variable, plus a few new instruction
encodings.

Part of #929
  • Loading branch information
fitzgen committed Jun 25, 2020
1 parent c6b0193 commit 13d3537
Show file tree
Hide file tree
Showing 20 changed files with 704 additions and 59 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 12 additions & 10 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,20 @@ fn ignore(testsuite: &str, testname: &str, strategy: &str) -> bool {
// testsuite repo.
("simd", "simd_const") => return true,

("reference_types", "table_copy_on_imported_tables")
| ("reference_types", "externref_id_function")
| ("reference_types", "table_size")
| ("reference_types", "simple_ref_is_null")
| ("reference_types", "table_grow_with_funcref") => {
// TODO(#1886): Ignore if this isn't x64, because Cranelift only
// supports reference types on x64.
return env::var("CARGO_CFG_TARGET_ARCH").unwrap() != "x86_64";
// Still working on implementing these. See #929.
("reference_types", "global")
| ("reference_types", "linking")
| ("reference_types", "ref_func")
| ("reference_types", "ref_null")
| ("reference_types", "table_fill") => {
return true;
}

// Still working on implementing these. See #929.
("reference_types", _) => return true,
// TODO(#1886): Ignore reference types tests if this isn't x64,
// because Cranelift only supports reference types on x64.
("reference_types", _) => {
return env::var("CARGO_CFG_TARGET_ARCH").unwrap() != "x86_64";
}

_ => {}
},
Expand Down
40 changes: 40 additions & 0 deletions cranelift/codegen/meta/src/isa/x86/encodings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,32 @@ impl PerCpuModeEncodings {
});
}

/// Add encodings for `inst.r32` to X86_32.
/// Add encodings for `inst.r32` to X86_64 with and without REX.
/// Add encodings for `inst.r64` to X86_64 with a REX.W prefix.
fn enc_r32_r64_instp(
&mut self,
inst: &Instruction,
template: Template,
instp: InstructionPredicateNode,
) {
self.enc32_func(inst.bind(R32), template.nonrex(), |builder| {
builder.inst_predicate(instp.clone())
});

// REX-less encoding must come after REX encoding so we don't use it by default. Otherwise
// reg-alloc would never use r8 and up.
self.enc64_func(inst.bind(R32), template.rex(), |builder| {
builder.inst_predicate(instp.clone())
});
self.enc64_func(inst.bind(R32), template.nonrex(), |builder| {
builder.inst_predicate(instp.clone())
});
self.enc64_func(inst.bind(R64), template.rex().w(), |builder| {
builder.inst_predicate(instp)
});
}

/// Add encodings for `inst.r32` to X86_32.
/// Add encodings for `inst.r64` to X86_64 with a REX.W prefix.
fn enc_r32_r64_rex_only(&mut self, inst: impl Into<InstSpec>, template: Template) {
Expand Down Expand Up @@ -810,6 +836,11 @@ fn define_memory(
recipe.opcodes(&MOV_LOAD),
is_load_complex_length_two.clone(),
);
e.enc_r32_r64_instp(
load_complex,
recipe.opcodes(&MOV_LOAD),
is_load_complex_length_two.clone(),
);
e.enc_x86_64_instp(
uload32_complex,
recipe.opcodes(&MOV_LOAD),
Expand Down Expand Up @@ -855,6 +886,11 @@ fn define_memory(
recipe.opcodes(&MOV_STORE),
is_store_complex_length_three.clone(),
);
e.enc_r32_r64_instp(
store_complex,
recipe.opcodes(&MOV_STORE),
is_store_complex_length_three.clone(),
);
e.enc_x86_64_instp(
istore32_complex,
recipe.opcodes(&MOV_STORE),
Expand Down Expand Up @@ -948,6 +984,10 @@ fn define_memory(
e.enc64_rec(fill_nop.bind(ty), rec_ffillnull, 0);
e.enc32_rec(fill_nop.bind(ty), rec_ffillnull, 0);
}
for &ty in &[R64, R32] {
e.enc64_rec(fill_nop.bind(ty), rec_fillnull, 0);
e.enc32_rec(fill_nop.bind(ty), rec_fillnull, 0);
}

// Load 32 bits from `b1`, `i8` and `i16` spill slots. See `spill.b1` above.

Expand Down
4 changes: 2 additions & 2 deletions cranelift/codegen/meta/src/shared/instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ fn define_control_flow(
let iAddr = &TypeVar::new(
"iAddr",
"An integer address type",
TypeSetBuilder::new().ints(32..64).build(),
TypeSetBuilder::new().ints(32..64).refs(32..64).build(),
);

{
Expand Down Expand Up @@ -744,7 +744,7 @@ pub(crate) fn define(
let iAddr = &TypeVar::new(
"iAddr",
"An integer address type",
TypeSetBuilder::new().ints(32..64).build(),
TypeSetBuilder::new().ints(32..64).refs(32..64).build(),
);

let Ref = &TypeVar::new(
Expand Down
6 changes: 5 additions & 1 deletion cranelift/codegen/src/binemit/relaxation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,11 @@ fn fallthroughs(func: &mut Function) {
Opcode::Fallthrough => {
// Somebody used a fall-through instruction before the branch relaxation pass.
// Make sure it is correct, i.e. the destination is the layout successor.
debug_assert_eq!(destination, succ, "Illegal fall-through in {}", block)
debug_assert_eq!(
destination, succ,
"Illegal fallthrough from {} to {}, but {}'s successor is {}",
block, destination, block, succ
)
}
Opcode::Jump => {
// If this is a jump to the successor block, change it to a fall-through.
Expand Down
4 changes: 2 additions & 2 deletions cranelift/codegen/src/ir/valueloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ impl ValueLoc {
pub fn unwrap_reg(self) -> RegUnit {
match self {
Self::Reg(ru) => ru,
_ => panic!("Expected register: {:?}", self),
_ => panic!("unwrap_reg expected register, found {:?}", self),
}
}

/// Get the stack slot of this location, or panic.
pub fn unwrap_stack(self) -> StackSlot {
match self {
Self::Stack(ss) => ss,
_ => panic!("Expected stack slot: {:?}", self),
_ => panic!("unwrap_stack expected stack slot, found {:?}", self),
}
}

Expand Down
6 changes: 5 additions & 1 deletion cranelift/codegen/src/postopt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,11 @@ fn optimize_complex_addresses(pos: &mut EncCursor, inst: Inst, isa: &dyn TargetI
}

let ok = pos.func.update_encoding(inst, isa).is_ok();
debug_assert!(ok);
debug_assert!(
ok,
"failed to update encoding for `{}`",
pos.func.dfg.display_inst(inst, isa)
);
}

//----------------------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion cranelift/codegen/src/redundant_reload_remover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,11 @@ impl RedundantReloadRemover {
// Load is completely redundant. Convert it to a no-op.
dfg.replace(inst).fill_nop(arg);
let ok = func.update_encoding(inst, isa).is_ok();
debug_assert!(ok, "fill_nop encoding missing for this type");
debug_assert!(
ok,
"fill_nop encoding missing for this type: `{}`",
func.dfg.display_inst(inst, isa)
);
}
Transform::ChangeToCopyToSSA(ty, reg) => {
// We already have the relevant value in some other register. Convert the
Expand Down
5 changes: 5 additions & 0 deletions cranelift/frontend/src/frontend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ impl<'a> FunctionBuilder<'a> {
}
}

/// Get the block that this builder is currently at.
pub fn current_block(&self) -> Option<Block> {
self.position.expand()
}

/// Set the source location that should be assigned to all new instructions.
pub fn set_srcloc(&mut self, srcloc: ir::SourceLoc) {
self.srcloc = srcloc;
Expand Down
9 changes: 2 additions & 7 deletions cranelift/wasm/src/code_translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1186,19 +1186,14 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
let table_index = TableIndex::from_u32(*index);
let table = state.get_or_create_table(builder.func, *index, environ)?;
let index = state.pop1();
state.push1(environ.translate_table_get(
builder.cursor(),
table_index,
table,
index,
)?);
state.push1(environ.translate_table_get(builder, table_index, table, index)?);
}
Operator::TableSet { table: index } => {
let table_index = TableIndex::from_u32(*index);
let table = state.get_or_create_table(builder.func, *index, environ)?;
let value = state.pop1();
let index = state.pop1();
environ.translate_table_set(builder.cursor(), table_index, table, value, index)?;
environ.translate_table_set(builder, table_index, table, value, index)?;
}
Operator::TableCopy {
dst_table: dst_table_index,
Expand Down
7 changes: 4 additions & 3 deletions cranelift/wasm/src/environ/dummy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use cranelift_codegen::ir::types::*;
use cranelift_codegen::ir::{self, InstBuilder};
use cranelift_codegen::isa::TargetFrontendConfig;
use cranelift_entity::{EntityRef, PrimaryMap, SecondaryMap};
use cranelift_frontend::FunctionBuilder;
use std::boxed::Box;
use std::string::String;
use std::vec::Vec;
Expand Down Expand Up @@ -452,17 +453,17 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ

fn translate_table_get(
&mut self,
mut pos: FuncCursor,
builder: &mut FunctionBuilder,
_table_index: TableIndex,
_table: ir::Table,
_index: ir::Value,
) -> WasmResult<ir::Value> {
Ok(pos.ins().null(self.reference_type()))
Ok(builder.ins().null(self.reference_type()))
}

fn translate_table_set(
&mut self,
_pos: FuncCursor,
_builder: &mut FunctionBuilder,
_table_index: TableIndex,
_table: ir::Table,
_value: ir::Value,
Expand Down
4 changes: 2 additions & 2 deletions cranelift/wasm/src/environ/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ pub trait FuncEnvironment: TargetEnvironment {
/// Translate a `table.get` WebAssembly instruction.
fn translate_table_get(
&mut self,
pos: FuncCursor,
builder: &mut FunctionBuilder,
table_index: TableIndex,
table: ir::Table,
index: ir::Value,
Expand All @@ -377,7 +377,7 @@ pub trait FuncEnvironment: TargetEnvironment {
/// Translate a `table.set` WebAssembly instruction.
fn translate_table_set(
&mut self,
pos: FuncCursor,
builder: &mut FunctionBuilder,
table_index: TableIndex,
table: ir::Table,
value: ir::Value,
Expand Down
4 changes: 3 additions & 1 deletion cranelift/wasm/src/sections_translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,9 @@ pub fn parse_element_section<'data>(
let index = ElemIndex::from_u32(index as u32);
environ.declare_passive_element(index, segments)?;
}
ElementKind::Declared => return Err(wasm_unsupported!("element kind declared")),
ElementKind::Declared => {
// Nothing to do here.
}
}
}
Ok(())
Expand Down
1 change: 1 addition & 0 deletions crates/environ/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ edition = "2018"
anyhow = "1.0"
cranelift-codegen = { path = "../../cranelift/codegen", version = "0.65.0", features = ["enable-serde"] }
cranelift-entity = { path = "../../cranelift/entity", version = "0.65.0", features = ["enable-serde"] }
cranelift-frontend = { path = "../../cranelift/frontend", version = "0.65.0" }
cranelift-wasm = { path = "../../cranelift/wasm", version = "0.65.0", features = ["enable-serde"] }
wasmparser = "0.57.0"
lightbeam = { path = "../lightbeam", optional = true, version = "0.18.0" }
Expand Down
Loading

0 comments on commit 13d3537

Please sign in to comment.