Skip to content

Commit

Permalink
Make VMOffset calculation more readable (#3793)
Browse files Browse the repository at this point in the history 
* Fix typo

* Move vmoffset field size and field name together

The previous code was quite confusing about what applied to which field.
The new code also makes it easier to move fields around and insert and
delete fields.

* Move builtin_functions before all variable sized fields

This allows the offset to be calculated at compile time

* Add cadd and cmul convenience functions

* Remove comment

* Change fields! syntax as per review

* Add implicit u32::from to fields!
  • Loading branch information
@bjorn3
bjorn3 authored Feb 22, 2022
1 parent 084452a commit bbd5277
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 100 deletions.
8 changes: 4 additions & 4 deletions crates/cranelift/src/func_environ.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
/// reference count.
///
/// The new reference count is returned.
fn mutate_extenref_ref_count(
fn mutate_externref_ref_count(
&mut self,
builder: &mut FunctionBuilder,
externref: ir::Value,
Expand Down Expand Up @@ -1036,7 +1036,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
// * store the reference into the bump table at `*next`,
// * and finally increment the `next` bump finger.
builder.switch_to_block(no_gc_block);
self.mutate_extenref_ref_count(builder, elem, 1);
self.mutate_externref_ref_count(builder, elem, 1);
builder.ins().store(ir::MemFlags::trusted(), elem, next, 0);

let new_next = builder
Expand Down Expand Up @@ -1159,7 +1159,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
.brnz(value_is_null, check_current_elem_block, &[]);
builder.ins().jump(inc_ref_count_block, &[]);
builder.switch_to_block(inc_ref_count_block);
self.mutate_extenref_ref_count(builder, value, 1);
self.mutate_externref_ref_count(builder, value, 1);
builder.ins().jump(check_current_elem_block, &[]);

// Grab the current element from the table, and store the new
Expand Down Expand Up @@ -1193,7 +1193,7 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
builder.ins().jump(continue_block, &[]);

builder.switch_to_block(dec_ref_count_block);
let prev_ref_count = self.mutate_extenref_ref_count(builder, current_elem, -1);
let prev_ref_count = self.mutate_externref_ref_count(builder, current_elem, -1);
let one = builder.ins().iconst(pointer_type, 1);
builder
.ins()
Expand Down
157 changes: 61 additions & 96 deletions crates/environ/src/vmoffsets.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// interrupts: *const VMInterrupts,
// externref_activations_table: *mut VMExternRefActivationsTable,
// store: *mut dyn Store,
// builtins: *mut VMBuiltinFunctionsArray,
// signature_ids: *const VMSharedSignatureIndex,
// imported_functions: [VMFunctionImport; module.num_imported_functions],
// imported_tables: [VMTableImport; module.num_imported_tables],
Expand All @@ -16,7 +17,6 @@
// memories: [VMMemoryDefinition; module.num_defined_memories],
// globals: [VMGlobalDefinition; module.num_defined_globals],
// anyfuncs: [VMCallerCheckedAnyfunc; module.num_imported_functions + module.num_defined_functions],
// builtins: *mut VMBuiltinFunctionsArray,
// }

use crate::{
Expand Down Expand Up @@ -74,6 +74,7 @@ pub struct VMOffsets<P> {
epoch_ptr: u32,
externref_activations_table: u32,
store: u32,
builtin_functions: u32,
signature_ids: u32,
imported_functions: u32,
imported_tables: u32,
Expand All @@ -83,7 +84,6 @@ pub struct VMOffsets<P> {
defined_memories: u32,
defined_globals: u32,
defined_anyfuncs: u32,
builtin_functions: u32,
size: u32,
}

Expand Down Expand Up @@ -172,6 +172,7 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
epoch_ptr: 0,
externref_activations_table: 0,
store: 0,
builtin_functions: 0,
signature_ids: 0,
imported_functions: 0,
imported_tables: 0,
Expand All @@ -181,103 +182,67 @@ impl<P: PtrSize> From<VMOffsetsFields<P>> for VMOffsets<P> {
defined_memories: 0,
defined_globals: 0,
defined_anyfuncs: 0,
builtin_functions: 0,
size: 0,
};

ret.interrupts = 0;
ret.epoch_ptr = ret
.interrupts
.checked_add(u32::from(ret.ptr.size()))
.unwrap();
ret.externref_activations_table = ret
.epoch_ptr
.checked_add(u32::from(ret.ptr.size()))
.unwrap();
ret.store = ret
.externref_activations_table
.checked_add(u32::from(ret.ptr.size()))
.unwrap();
ret.signature_ids = ret
.store
.checked_add(u32::from(ret.ptr.size() * 2))
.unwrap();
ret.imported_functions = ret
.signature_ids
.checked_add(u32::from(ret.ptr.size()))
.unwrap();
ret.imported_tables = ret
.imported_functions
.checked_add(
ret.num_imported_functions
.checked_mul(u32::from(ret.size_of_vmfunction_import()))
.unwrap(),
)
.unwrap();
ret.imported_memories = ret
.imported_tables
.checked_add(
ret.num_imported_tables
.checked_mul(u32::from(ret.size_of_vmtable_import()))
.unwrap(),
)
.unwrap();
ret.imported_globals = ret
.imported_memories
.checked_add(
ret.num_imported_memories
.checked_mul(u32::from(ret.size_of_vmmemory_import()))
.unwrap(),
)
.unwrap();
ret.defined_tables = ret
.imported_globals
.checked_add(
ret.num_imported_globals
.checked_mul(u32::from(ret.size_of_vmglobal_import()))
.unwrap(),
)
.unwrap();
ret.defined_memories = ret
.defined_tables
.checked_add(
ret.num_defined_tables
.checked_mul(u32::from(ret.size_of_vmtable_definition()))
.unwrap(),
)
.unwrap();
ret.defined_globals = align(
ret.defined_memories
.checked_add(
ret.num_defined_memories
.checked_mul(u32::from(ret.size_of_vmmemory_definition()))
.unwrap(),
)
.unwrap(),
16,
);
ret.defined_anyfuncs = ret
.defined_globals
.checked_add(
ret.num_defined_globals
.checked_mul(u32::from(ret.size_of_vmglobal_definition()))
.unwrap(),
)
.unwrap();
ret.builtin_functions = ret
.defined_anyfuncs
.checked_add(
ret.num_imported_functions
.checked_add(ret.num_defined_functions)
.unwrap()
.checked_mul(u32::from(ret.size_of_vmcaller_checked_anyfunc()))
.unwrap(),
)
.unwrap();
ret.size = ret
.builtin_functions
.checked_add(u32::from(ret.pointer_size()))
.unwrap();
// Convenience functions for checked addition and multiplication.
// As side effect this reduces binary size by using only a single
// `#[track_caller]` location for each function instead of one for
// each individual invocation.
#[inline]
fn cadd(count: u32, size: u32) -> u32 {
count.checked_add(size).unwrap()
}

#[inline]
fn cmul(count: u32, size: u8) -> u32 {
count.checked_mul(u32::from(size)).unwrap()
}

let mut next_field_offset = 0;

macro_rules! fields {
(size($field:ident) = $size:expr, $($rest:tt)*) => {
ret.$field = next_field_offset;
next_field_offset = cadd(next_field_offset, u32::from($size));
fields!($($rest)*);
};
(align($align:literal), $($rest:tt)*) => {
next_field_offset = align(next_field_offset, $align);
fields!($($rest)*);
};
() => {};
}

fields! {
size(interrupts) = ret.ptr.size(),
size(epoch_ptr) = ret.ptr.size(),
size(externref_activations_table) = ret.ptr.size(),
size(store) = ret.ptr.size() * 2,
size(builtin_functions) = ret.pointer_size(),
size(signature_ids) = ret.ptr.size(),
size(imported_functions)
= cmul(ret.num_imported_functions, ret.size_of_vmfunction_import()),
size(imported_tables)
= cmul(ret.num_imported_tables, ret.size_of_vmtable_import()),
size(imported_memories)
= cmul(ret.num_imported_memories, ret.size_of_vmmemory_import()),
size(imported_globals)
= cmul(ret.num_imported_globals, ret.size_of_vmglobal_import()),
size(defined_tables)
= cmul(ret.num_defined_tables, ret.size_of_vmtable_definition()),
size(defined_memories)
= cmul(ret.num_defined_memories, ret.size_of_vmmemory_definition()),
align(16),
size(defined_globals)
= cmul(ret.num_defined_globals, ret.size_of_vmglobal_definition()),
size(defined_anyfuncs) = cmul(
cadd(ret.num_imported_functions, ret.num_defined_functions),
ret.size_of_vmcaller_checked_anyfunc(),
),
}

ret.size = next_field_offset;

return ret;
}
Expand Down

0 comments on commit bbd5277

Please sign in to comment.