Skip to content

YJIT: Allow calling iseq with mixed optional and keyword args and fix required kwarg check #5285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 17, 2021
Merged

YJIT: Allow calling iseq with mixed optional and keyword args and fix required kwarg check #5285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4943ee1
YJIT: Allow iseq with both opt and kwargs
jhawthorn Dec 16, 2021
853bd64
YJIT: Fix check for required kwargs
jhawthorn Dec 16, 2021
c3e5ee2
YJIT: Remove double check for block arg handling
XrXr Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions bootstraptest/test_yjit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2226,6 +2226,22 @@ def kwargs(value:)
5.times.map { kwargs(value: 1) }.uniq
}

assert_equal '[:ok]', %q{
def kwargs(value:)
value
end

5.times.map { kwargs() rescue :ok }.uniq
}

assert_equal '[:ok]', %q{
def kwargs(a:, b: nil)
value
end

5.times.map { kwargs(b: 123) rescue :ok }.uniq
}

assert_equal '[[1, 2]]', %q{
def kwargs(left:, right:)
[left, right]
Expand All @@ -2247,6 +2263,24 @@ def kwargs(lead, kwarg:)
5.times.map { kwargs(1, kwarg: 2) }.uniq
}

# optional and keyword args
assert_equal '[[1, 2, 3]]', %q{
def opt_and_kwargs(a, b=2, c: nil)
[a,b,c]
end

5.times.map { opt_and_kwargs(1, c: 3) }.uniq
}

assert_equal '[[1, 2, 3]]', %q{
def opt_and_kwargs(a, b=nil, c: nil)
[a,b,c]
end

5.times.map { opt_and_kwargs(1, 2, c: 3) }.uniq
}


# leading and keyword arguments are swapped into the right order
assert_equal '[[1, 2, 3, 4, 5, 6]]', %q{
def kwargs(five, six, a:, b:, c:, d:)
Expand Down
11 changes: 11 additions & 0 deletions test/ruby/test_yjit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,17 @@ def fib(n)
RUBY
end

def test_optarg_and_kwarg
assert_no_exits(<<~'RUBY')
def opt_and_kwarg(a, b=nil, c: nil)
end

2.times do
opt_and_kwarg(1, 2, c: 3)
end
RUBY
end

def test_ctx_different_mappings
# regression test simplified from URI::Generic#hostname=
assert_compiles(<<~'RUBY', frozen_string_literal: true)
Expand Down
166 changes: 76 additions & 90 deletions yjit_codegen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3440,25 +3440,6 @@ gen_return_branch(codeblock_t *cb, uint8_t *target0, uint8_t *target1, uint8_t s
}
}

// Returns whether the iseq only needs positional (lead) argument setup.
static bool
iseq_lead_only_arg_setup_p(const rb_iseq_t *iseq)
{
// When iseq->body->local_iseq == iseq, setup_parameters_complex()
// doesn't do anything to setup the block parameter.
bool takes_block = iseq->body->param.flags.has_block;
return (!takes_block || iseq->body->local_iseq == iseq) &&
iseq->body->param.flags.has_opt == false &&
iseq->body->param.flags.has_rest == false &&
iseq->body->param.flags.has_post == false &&
iseq->body->param.flags.has_kw == false &&
iseq->body->param.flags.has_kwrest == false &&
iseq->body->param.flags.accepts_no_kwarg == false;
}

bool rb_iseq_only_optparam_p(const rb_iseq_t *iseq);
bool rb_iseq_only_kwparam_p(const rb_iseq_t *iseq);

// If true, the iseq is leaf and it can be replaced by a single C call.
static bool
rb_leaf_invokebuiltin_iseq_p(const rb_iseq_t *iseq)
Expand Down Expand Up @@ -3492,78 +3473,100 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r
// specified at the call site. We need to keep track of the fact that this
// value is present on the stack in order to properly set up the callee's
// stack pointer.
bool doing_kw_call = false;
const bool doing_kw_call = iseq->body->param.flags.has_kw;
const bool supplying_kws = vm_ci_flag(ci) & VM_CALL_KWARG;

if (vm_ci_flag(ci) & VM_CALL_TAILCALL) {
// We can't handle tailcalls
GEN_COUNTER_INC(cb, send_iseq_tailcall);
return YJIT_CANT_COMPILE;
}

// Arity handling and optional parameter setup
int num_params = iseq->body->param.size;
uint32_t start_pc_offset = 0;
// No support for callees with these parameters yet as they require allocation
// or complex handling.
if (iseq->body->param.flags.has_rest ||
iseq->body->param.flags.has_post ||
iseq->body->param.flags.has_kwrest) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

if (iseq_lead_only_arg_setup_p(iseq)) {
// If we have keyword arguments being passed to a callee that only takes
// positionals, then we need to allocate a hash. For now we're going to
// call that too complex and bail.
if (vm_ci_flag(ci) & VM_CALL_KWARG) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}
// If we have keyword arguments being passed to a callee that only takes
// positionals, then we need to allocate a hash. For now we're going to
// call that too complex and bail.
if (supplying_kws && !iseq->body->param.flags.has_kw) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

// If we have a method accepting no kwargs (**nil), exit if we have passed
// it any kwargs.
if (supplying_kws && iseq->body->param.flags.accepts_no_kwarg) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

num_params = iseq->body->param.lead_num;
// For computing number of locals to setup for the callee
int num_params = iseq->body->param.size;

if (num_params != argc) {
GEN_COUNTER_INC(cb, send_iseq_arity_error);
return YJIT_CANT_COMPILE;
// Block parameter handling. This mirrors setup_parameters_complex().
if (iseq->body->param.flags.has_block) {
if (iseq->body->local_iseq == iseq) {
// Block argument is passed through EP and not setup as a local in
// the callee.
num_params--;
}
}
else if (rb_iseq_only_optparam_p(iseq)) {
// If we have keyword arguments being passed to a callee that only takes
// positionals and optionals, then we need to allocate a hash. For now
// we're going to call that too complex and bail.
if (vm_ci_flag(ci) & VM_CALL_KWARG) {
else {
// In this case (param.flags.has_block && local_iseq != iseq),
// the block argument is setup as a local variable and requires
// materialization (allocation). Bail.
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}
}

// 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));
uint32_t start_pc_offset = 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;
const int required_num = iseq->body->param.lead_num;

if (opts_filled < 0 || opts_filled > opt_num) {
GEN_COUNTER_INC(cb, send_iseq_arity_error);
return YJIT_CANT_COMPILE;
}
// This struct represents the metadata about the caller-specified
// keyword arguments.
const struct rb_callinfo_kwarg *kw_arg = vm_ci_kwarg(ci);
const int kw_arg_num = kw_arg ? kw_arg->keyword_len : 0;

// Arity handling and optional parameter setup
const int opts_filled = argc - required_num - kw_arg_num;
const int opt_num = iseq->body->param.opt_num;
const int opts_missing = opt_num - opts_filled;

if (opts_filled < 0 || opts_filled > opt_num) {
GEN_COUNTER_INC(cb, send_iseq_arity_error);
return YJIT_CANT_COMPILE;
}

// If we have unfilled optional arguments and keyword arguments then we
// would need to move adjust the arguments location to account for that.
// For now we aren't handling this case.
if (doing_kw_call && opts_missing > 0) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

if (opt_num > 0) {
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)) {
const int lead_num = iseq->body->param.lead_num;

doing_kw_call = true;

if (doing_kw_call) {
// Here we're calling a method with keyword arguments and specifying
// keyword arguments at this call site.

// This struct represents the metadata about the caller-specified
// keyword arguments.
const struct rb_callinfo_kwarg *kw_arg = vm_ci_kwarg(ci);

// This struct represents the metadata about the callee-specified
// keyword parameters.
const struct rb_iseq_param_keyword *keyword = iseq->body->param.keyword;

int required_kwargs_filled = 0;

if (keyword->num > 30) {
// We have so many keywords that (1 << num) encoded as a FIXNUM
// (which shifts it left one more) no longer fits inside a 32-bit
Expand All @@ -3572,13 +3575,8 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r
return YJIT_CANT_COMPILE;
}

if (vm_ci_flag(ci) & VM_CALL_KWARG) {
// Check that the size of non-keyword arguments matches
if (lead_num != argc - kw_arg->keyword_len) {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

// Check that the kwargs being passed are valid
if (supplying_kws) {
// This is the list of keyword arguments that the callee specified
// in its initial declaration.
const ID *callee_kwargs = keyword->table;
Expand Down Expand Up @@ -3611,32 +3609,20 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r
GEN_COUNTER_INC(cb, send_iseq_kwargs_mismatch);
return YJIT_CANT_COMPILE;
}

// Keep a count to ensure all required kwargs are specified
if (callee_idx < keyword->required_num) {
required_kwargs_filled++;
}
}
}
else if (argc == lead_num) {
// Here we are calling a method that accepts keyword arguments
// (optional or required) but we're not passing any keyword
// arguments at this call site

if (keyword->required_num != 0) {
// If any of the keywords are required this is a mismatch
GEN_COUNTER_INC(cb, send_iseq_kwargs_mismatch);
return YJIT_CANT_COMPILE;
}

doing_kw_call = true;
}
else {
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
RUBY_ASSERT(required_kwargs_filled <= keyword->required_num);
if (required_kwargs_filled != keyword->required_num) {
GEN_COUNTER_INC(cb, send_iseq_kwargs_mismatch);
return YJIT_CANT_COMPILE;
}
}
else {
// Only handle iseqs that have simple parameter setup.
// See vm_callee_setup_arg().
GEN_COUNTER_INC(cb, send_iseq_complex_callee);
return YJIT_CANT_COMPILE;
}

// Number of locals that are not parameters
const int num_locals = iseq->body->local_table_size - num_params;
Expand Down Expand Up @@ -3716,7 +3702,7 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r
ID *caller_kwargs = ALLOCA_N(VALUE, total_kwargs);
int kwarg_idx;
for (kwarg_idx = 0; kwarg_idx < caller_keyword_len; kwarg_idx++) {
caller_kwargs[kwarg_idx] = SYM2ID(caller_keywords[kwarg_idx]);
caller_kwargs[kwarg_idx] = SYM2ID(caller_keywords[kwarg_idx]);
}

int unspecified_bits = 0;
Expand Down