Skip to content

Commit

Permalink
Implement calls to methods with simple optional params
Browse files Browse the repository at this point in the history
* 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%)
```
  • Loading branch information
XrXr committed Oct 20, 2021
1 parent 9482fdb commit 3cc5335
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 24 deletions.
36 changes: 36 additions & 0 deletions bootstraptest/test_yjit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
4 changes: 2 additions & 2 deletions vm_insnhelper.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -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 &&
Expand Down
75 changes: 55 additions & 20 deletions yjit_codegen.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
*/
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions yjit_iface.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 3cc5335

Please sign in to comment.