From 3cc5335ebc8a7c98ed15c5107ec8b64046ea3d0e Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Wed, 28 Apr 2021 12:59:26 -0400 Subject: [PATCH] Implement calls to methods with simple optional params * Implement calls to methods with simple optional params * Remove unnecessary MJIT_STATIC See comment for MJIT_STATIC. I added it not knowing whether it's required because the function next to it has it. Don't use it and wait for problems to come up instead. * Better naming, some comments * Count bailing on kw only iseqs On railsbench: ``` opt_send_without_block exit reasons: bmethod 59729 (27.7%) optimized_method 59137 (27.5%) iseq_complex_callee 41362 (19.2%) alias_method 33346 (15.5%) callsite_not_simple 19170 ( 8.9%) iseq_only_keywords 1300 ( 0.6%) kw_splat 1299 ( 0.6%) cfunc_ruby_array_varg 18 ( 0.0%) ``` --- bootstraptest/test_yjit.rb | 36 ++++++++++++++++++ vm_insnhelper.c | 4 +- yjit_codegen.c | 75 ++++++++++++++++++++++++++++---------- yjit_iface.h | 5 ++- 4 files changed, 96 insertions(+), 24 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index d02506b2b864a2..6bf0f53e9eaf71 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -736,3 +736,39 @@ class Symbol [dyn_sym.get_foo, sym.get_foo] } + +# passing too few arguments to method with optional parameters +assert_equal 'raised', %q{ + def opt(a, b = 0) + end + + def use + opt + end + + use rescue nil + begin + use + :ng + rescue ArgumentError + :raised + end +} + +# passing too many arguments to method with optional parameters +assert_equal 'raised', %q{ + def opt(a, b = 0) + end + + def use + opt(1, 2, 3, 4) + end + + use rescue nil + begin + use + :ng + rescue ArgumentError + :raised + end +} diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 00b352df3dda95..124de73afb6f03 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -2228,7 +2228,7 @@ rb_simple_iseq_p(const rb_iseq_t *iseq) iseq->body->param.flags.has_block == FALSE; } -static bool +bool rb_iseq_only_optparam_p(const rb_iseq_t *iseq) { return iseq->body->param.flags.has_opt == TRUE && @@ -2240,7 +2240,7 @@ rb_iseq_only_optparam_p(const rb_iseq_t *iseq) iseq->body->param.flags.has_block == FALSE; } -static bool +bool rb_iseq_only_kwparam_p(const rb_iseq_t *iseq) { return iseq->body->param.flags.has_opt == FALSE && diff --git a/yjit_codegen.c b/yjit_codegen.c index c545024b1600ef..f02c5196125d1c 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -567,7 +567,8 @@ gen_putself(jitstate_t* jit, ctx_t* ctx) } // Compute the index of a local variable from its slot index -uint32_t slot_to_local_idx(const rb_iseq_t *iseq, int32_t slot_idx) +static uint32_t +slot_to_local_idx(const rb_iseq_t *iseq, int32_t slot_idx) { // Convoluted rules from local_var_name() in iseq.c int32_t local_table_size = iseq->body->local_table_size; @@ -633,10 +634,10 @@ gen_setlocal_wc0(jitstate_t* jit, ctx_t* ctx) { VALUE flags = ep[VM_ENV_DATA_INDEX_FLAGS]; if (LIKELY((flags & VM_ENV_FLAG_WB_REQUIRED) == 0)) { - VM_STACK_ENV_WRITE(ep, index, v); + VM_STACK_ENV_WRITE(ep, index, v); } else { - vm_env_write_slowpath(ep, index, v); + vm_env_write_slowpath(ep, index, v); } } */ @@ -1772,8 +1773,6 @@ gen_oswb_cfunc(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const return YJIT_END_BLOCK; } -bool rb_simple_iseq_p(const rb_iseq_t *iseq); - static void gen_return_branch(codeblock_t* cb, uint8_t* target0, uint8_t* target1, uint8_t shape) { @@ -1791,31 +1790,67 @@ gen_return_branch(codeblock_t* cb, uint8_t* target0, uint8_t* target1, uint8_t s } } +bool rb_simple_iseq_p(const rb_iseq_t *iseq); +bool rb_iseq_only_optparam_p(const rb_iseq_t *iseq); +bool rb_iseq_only_kwparam_p(const rb_iseq_t *iseq); + static codegen_status_t -gen_oswb_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const rb_callable_method_entry_t *cme, int32_t argc) +gen_oswb_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const rb_callable_method_entry_t *cme, const int32_t argc) { const rb_iseq_t *iseq = def_iseq_ptr(cme->def); - const VALUE* start_pc = iseq->body->iseq_encoded; - int num_params = iseq->body->param.size; - int num_locals = iseq->body->local_table_size - num_params; - if (num_params != argc) { - GEN_COUNTER_INC(cb, oswb_iseq_argc_mismatch); + if (vm_ci_flag(ci) & VM_CALL_TAILCALL) { + // We can't handle tailcalls + GEN_COUNTER_INC(cb, oswb_iseq_tailcall); return YJIT_CANT_COMPILE; } - if (!rb_simple_iseq_p(iseq)) { - // Only handle iseqs that have simple parameters. - // See vm_callee_setup_arg(). - GEN_COUNTER_INC(cb, oswb_iseq_not_simple); - return YJIT_CANT_COMPILE; + // Arity handling and optional parameter setup + int num_params = iseq->body->param.size; + uint32_t start_pc_offset = 0; + if (rb_simple_iseq_p(iseq)) { + if (num_params != argc) { + GEN_COUNTER_INC(cb, oswb_iseq_arity_error); + return YJIT_CANT_COMPILE; + } } + else if (rb_iseq_only_optparam_p(iseq)) { + // These are iseqs with 0 or more required parameters followed by 1 + // or more optional parameters. + // We follow the logic of vm_call_iseq_setup_normal_opt_start() + // and these are the preconditions required for using that fast path. + RUBY_ASSERT(vm_ci_markable(ci) && ((vm_ci_flag(ci) & + (VM_CALL_KW_SPLAT | VM_CALL_KWARG | VM_CALL_ARGS_SPLAT)) == 0)); + + const int required_num = iseq->body->param.lead_num; + const int opts_filled = argc - required_num; + const int opt_num = iseq->body->param.opt_num; + + if (opts_filled < 0 || opts_filled > opt_num) { + GEN_COUNTER_INC(cb, oswb_iseq_arity_error); + return YJIT_CANT_COMPILE; + } - if (vm_ci_flag(ci) & VM_CALL_TAILCALL) { - // We can't handle tailcalls - GEN_COUNTER_INC(cb, oswb_iseq_tailcall); + num_params -= opt_num - opts_filled; + start_pc_offset = (uint32_t)iseq->body->param.opt_table[opts_filled]; + } + else if (rb_iseq_only_kwparam_p(iseq)) { + // vm_callee_setup_arg() has a fast path for this. + GEN_COUNTER_INC(cb, oswb_iseq_only_keywords); return YJIT_CANT_COMPILE; } + else { + // Only handle iseqs that have simple parameter setup. + // See vm_callee_setup_arg(). + GEN_COUNTER_INC(cb, oswb_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + + // The starting pc of the callee frame + const VALUE *start_pc = &iseq->body->iseq_encoded[start_pc_offset]; + + // Number of locals that are not parameters + const int num_locals = iseq->body->local_table_size - num_params; // Create a size-exit to fall back to the interpreter uint8_t* side_exit = yjit_side_exit(jit, ctx); @@ -1934,7 +1969,7 @@ gen_oswb_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r gen_direct_jump( jit->block, &callee_ctx, - (blockid_t){ iseq, 0 } + (blockid_t){ iseq, start_pc_offset } ); return true; diff --git a/yjit_iface.h b/yjit_iface.h index eabaff19554f78..a7a89b7c1f5ffc 100644 --- a/yjit_iface.h +++ b/yjit_iface.h @@ -38,8 +38,9 @@ YJIT_DECLARE_COUNTERS( oswb_cfunc_argc_mismatch, oswb_cfunc_toomany_args, oswb_iseq_tailcall, - oswb_iseq_argc_mismatch, - oswb_iseq_not_simple, + oswb_iseq_arity_error, + oswb_iseq_only_keywords, + oswb_iseq_complex_callee, oswb_not_implemented_method, oswb_getter_arity, oswb_se_receiver_not_heap,