From c1fe0e121bebb936cddeef142aad50e8864bf59d Mon Sep 17 00:00:00 2001 From: Brendan Hansen Date: Fri, 8 Jul 2022 16:05:31 -0500 Subject: [PATCH] bugfixes and performance improvements --- include/vm_codebuilder.h | 5 ++++- src/vm/code_builder.c | 39 ++++++++++++++++++++++++++++++++++++++- src/wasm/instance.c | 11 +++++------ src/wasm/module_parsing.h | 22 ++++++---------------- 4 files changed, 53 insertions(+), 24 deletions(-) diff --git a/include/vm_codebuilder.h b/include/vm_codebuilder.h index 5261fad..6ecadad 100644 --- a/include/vm_codebuilder.h +++ b/include/vm_codebuilder.h @@ -51,19 +51,21 @@ struct branch_patch_t { i32 label_idx; i32 static_arr; i32 static_idx; + bool targets_else; }; ovm_code_builder_t ovm_code_builder_new(ovm_program_t *program, i32 param_count, i32 local_count); label_target_t ovm_code_builder_wasm_target_idx(ovm_code_builder_t *builder, i32 idx); i32 ovm_code_builder_push_label_target(ovm_code_builder_t *builder, label_kind_t kind); void ovm_code_builder_pop_label_target(ovm_code_builder_t *builder); +void ovm_code_builder_patch_else(ovm_code_builder_t *builder, label_target_t if_target); void ovm_code_builder_free(ovm_code_builder_t *builder); void ovm_code_builder_add_nop(ovm_code_builder_t *builder); void ovm_code_builder_add_binop(ovm_code_builder_t *builder, u32 instr); void ovm_code_builder_add_unop(ovm_code_builder_t *builder, u32 instr); void ovm_code_builder_add_imm(ovm_code_builder_t *builder, u32 ovm_type, void *imm); void ovm_code_builder_add_branch(ovm_code_builder_t *builder, i32 label_idx); -void ovm_code_builder_add_cond_branch(ovm_code_builder_t *builder, i32 label_idx, bool branch_if_true); +void ovm_code_builder_add_cond_branch(ovm_code_builder_t *builder, i32 label_idx, bool branch_if_true, bool targets_else); void ovm_code_builder_add_branch_table(ovm_code_builder_t *builder, i32 count, i32 *label_indicies, i32 default_label_idx); void ovm_code_builder_add_return(ovm_code_builder_t *builder); void ovm_code_builder_add_call(ovm_code_builder_t *builder, i32 func_idx, i32 param_count, bool has_return_value); @@ -71,6 +73,7 @@ void ovm_code_builder_add_indirect_call(ovm_code_builder_t *builde void ovm_code_builder_drop_value(ovm_code_builder_t *builder); void ovm_code_builder_add_local_get(ovm_code_builder_t *builder, i32 local_idx); void ovm_code_builder_add_local_set(ovm_code_builder_t *builder, i32 local_idx); +void ovm_code_builder_add_local_tee(ovm_code_builder_t *builder, i32 local_idx); void ovm_code_builder_add_register_get(ovm_code_builder_t *builder, i32 local_idx); void ovm_code_builder_add_register_set(ovm_code_builder_t *builder, i32 local_idx); void ovm_code_builder_add_load(ovm_code_builder_t *builder, u32 ovm_type, i32 offset); diff --git a/src/vm/code_builder.c b/src/vm/code_builder.c index fa252fa..0ed8ba6 100644 --- a/src/vm/code_builder.c +++ b/src/vm/code_builder.c @@ -101,6 +101,24 @@ void ovm_code_builder_pop_label_target(ovm_code_builder_t *builder) { } } +void ovm_code_builder_patch_else(ovm_code_builder_t *builder, label_target_t if_target) { + assert(if_target.kind == label_kind_if); + + fori (i, 0, bh_arr_length(builder->branch_patches)) { + branch_patch_t patch = builder->branch_patches[i]; + if (patch.label_idx != if_target.idx) continue; + if (!patch.targets_else) continue; + + int br_delta = bh_arr_length(builder->program->code) - patch.branch_instr - 1; + assert(patch.kind == branch_patch_instr_a); + + builder->program->code[patch.branch_instr].a = br_delta; + + bh_arr_fastdelete(builder->branch_patches, i); + return; + } +} + void ovm_code_builder_add_nop(ovm_code_builder_t *builder) { ovm_instr_t nop = {0}; nop.full_instr = OVMI_NOP; @@ -163,13 +181,14 @@ void ovm_code_builder_add_branch(ovm_code_builder_t *builder, i32 label_idx) { patch.kind = branch_patch_instr_a; patch.branch_instr = bh_arr_length(builder->program->code); patch.label_idx = label_idx; + patch.targets_else = false; bh_arr_push(builder->branch_patches, patch); ovm_program_add_instructions(builder->program, 1, &branch_instr); } -void ovm_code_builder_add_cond_branch(ovm_code_builder_t *builder, i32 label_idx, bool branch_if_true) { +void ovm_code_builder_add_cond_branch(ovm_code_builder_t *builder, i32 label_idx, bool branch_if_true, bool targets_else) { ovm_instr_t branch_instr = {0}; if (branch_if_true) { branch_instr.full_instr = OVMI_BR_NZ; @@ -184,6 +203,7 @@ void ovm_code_builder_add_cond_branch(ovm_code_builder_t *builder, i32 label_idx patch.kind = branch_patch_instr_a; patch.branch_instr = bh_arr_length(builder->program->code); patch.label_idx = label_idx; + patch.targets_else = targets_else; bh_arr_push(builder->branch_patches, patch); @@ -232,6 +252,7 @@ void ovm_code_builder_add_branch_table(ovm_code_builder_t *builder, i32 count, i patch.label_idx = label_indicies[i]; patch.static_arr = table_idx; patch.static_idx = i; + patch.targets_else = false; bh_arr_push(builder->branch_patches, patch); } @@ -239,6 +260,7 @@ void ovm_code_builder_add_branch_table(ovm_code_builder_t *builder, i32 count, i default_patch.kind = branch_patch_instr_a; default_patch.branch_instr = bh_arr_length(builder->program->code) + 2; default_patch.label_idx = default_label_idx; + default_patch.targets_else = false; bh_arr_push(builder->branch_patches, default_patch); ovm_program_add_instructions(builder->program, 5, instrs); @@ -351,6 +373,21 @@ void ovm_code_builder_add_local_set(ovm_code_builder_t *builder, i32 local_idx) ovm_program_add_instructions(builder->program, 1, &instr); } +void ovm_code_builder_add_local_tee(ovm_code_builder_t *builder, i32 local_idx) { + ovm_instr_t instr = {0}; + instr.full_instr = OVMI_MOV; + instr.r = local_idx; // This makes the assumption that the params will be in + // the lower "address space" of the value numbers. This + // will be true for web assembly, because that's how it + // it was spec'd; but in the future for other things, + // this will be incorrect. + instr.a = POP_VALUE(builder); + + ovm_program_add_instructions(builder->program, 1, &instr); + + PUSH_VALUE(builder, instr.a); +} + void ovm_code_builder_add_register_get(ovm_code_builder_t *builder, i32 reg_idx) { ovm_instr_t instr = {0}; instr.full_instr = OVMI_REG_GET; diff --git a/src/wasm/instance.c b/src/wasm/instance.c index 3300a7f..916c29b 100644 --- a/src/wasm/instance.c +++ b/src/wasm/instance.c @@ -17,6 +17,7 @@ struct ovm_wasm_binding { int param_count; int result_count; wasm_func_t *func; + wasm_val_vec_t param_buffer; }; #define WASM_TO_OVM(w, o) { \ @@ -102,12 +103,8 @@ static wasm_trap_t *wasm_to_ovm_func_call_binding(void *vbinding, const wasm_val static void ovm_to_wasm_func_call_binding(void *env, ovm_value_t* params, ovm_value_t *res) { ovm_wasm_binding *binding = (ovm_wasm_binding *) env; - wasm_val_vec_t wasm_params; - wasm_params.data = alloca(sizeof(wasm_val_t) * binding->param_count); - wasm_params.size = binding->param_count; - fori (i, 0, binding->param_count) { - OVM_TO_WASM(params[i], wasm_params.data[i]); + OVM_TO_WASM(params[i], binding->param_buffer.data[i]); } wasm_val_t return_value; @@ -115,7 +112,7 @@ static void ovm_to_wasm_func_call_binding(void *env, ovm_value_t* params, ovm_va wasm_results.data = &return_value; wasm_results.size = binding->result_count; - wasm_trap_t *trap = wasm_func_call(binding->func, &wasm_params, &wasm_results); + wasm_trap_t *trap = wasm_func_call(binding->func, &binding->param_buffer, &wasm_results); assert(!trap); if (binding->result_count > 0) { @@ -162,6 +159,8 @@ static void prepare_instance(wasm_instance_t *instance, const wasm_extern_vec_t binding->param_count = functype->params.size; binding->result_count = functype->results.size; binding->func = func; + binding->param_buffer.data = bh_alloc(ovm_store->arena_allocator, sizeof(wasm_val_t) * binding->param_count); + binding->param_buffer.size = binding->param_count; ovm_state_register_external_func(ovm_state, importtype->external_func_idx, ovm_to_wasm_func_call_binding, binding); break; diff --git a/src/wasm/module_parsing.h b/src/wasm/module_parsing.h index 7ee0fce..61d8101 100644 --- a/src/wasm/module_parsing.h +++ b/src/wasm/module_parsing.h @@ -497,23 +497,14 @@ static void parse_instruction(build_context *ctx) { // // This uses the pattern of "branch if zero" to skip a section of // code if the condition was not true. - ovm_code_builder_add_cond_branch(&ctx->builder, if_target, false); + ovm_code_builder_add_cond_branch(&ctx->builder, if_target, false, true); break; } case 0x05: { - // - // HACK HACK HACK - // I'm not entirely happy with the way that this happens, but let me explain. - // When trying to make an "else", you need to have a unconditional branch that - // skips the alternate code path. This branch instruction both needs to know - // the label index of the else statment, and be part of the existing block. - // Therefore, this "peeks" at what the next label index will be, so it can - // know what label to track for where to jump. It feels like there should be - // a better way to do this. - ovm_code_builder_add_branch(&ctx->builder, ctx->builder.next_label_idx); - ovm_code_builder_pop_label_target(&ctx->builder); - ovm_code_builder_push_label_target(&ctx->builder, label_kind_block); + label_target_t if_target = ovm_code_builder_wasm_target_idx(&ctx->builder, 0); + ovm_code_builder_add_branch(&ctx->builder, if_target.idx); + ovm_code_builder_patch_else(&ctx->builder, if_target); break; } @@ -534,7 +525,7 @@ static void parse_instruction(build_context *ctx) { int label_idx = uleb128_to_uint(ctx->binary.data, &ctx->offset); label_target_t target = ovm_code_builder_wasm_target_idx(&ctx->builder, label_idx); - ovm_code_builder_add_cond_branch(&ctx->builder, target.idx, true); + ovm_code_builder_add_cond_branch(&ctx->builder, target.idx, true, false); break; } @@ -605,8 +596,7 @@ static void parse_instruction(build_context *ctx) { case 0x22: { int local_idx = uleb128_to_uint(ctx->binary.data, &ctx->offset); - ovm_code_builder_add_local_set(&ctx->builder, local_idx); - ovm_code_builder_add_local_get(&ctx->builder, local_idx); + ovm_code_builder_add_local_tee(&ctx->builder, local_idx); break; } -- 2.25.1