ruby-changes:70287
From: John <ko1@a...>
Date: Sat, 18 Dec 2021 08:26:19 +0900 (JST)
Subject: [ruby-changes:70287] 83aa68447c (master): YJIT: Allow iseq with both opt and kwargs
https://git.ruby-lang.org/ruby.git/commit/?id=83aa68447c From 83aa68447c87169b3610b6e04abebdcc592f0c16 Mon Sep 17 00:00:00 2001 From: John Hawthorn <john@h...> Date: Thu, 16 Dec 2021 10:19:24 -0800 Subject: YJIT: Allow iseq with both opt and kwargs Previously we mirrored the fast paths the interpreter had for having only one of kwargs or optional args. This commit aims to combine the cases and reduce complexity. Though this allows calling iseqs which have have both optional and keyword arguments, it requires that all optional arguments are specified when there are keyword arguments, since unspecified optional arguments appear before the kwargs. Support for this can be added a in a future PR. --- bootstraptest/test_yjit.rb | 26 ++++++++ test/ruby/test_yjit.rb | 11 ++++ yjit_codegen.c | 158 +++++++++++++++++++-------------------------- 3 files changed, 103 insertions(+), 92 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 0b2b78ca4a9..501f00d5b34 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -2226,6 +2226,14 @@ assert_equal '[1]', %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_yjit.rb#L2226 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 '[[1, 2]]', %q{ def kwargs(left:, right:) [left, right] @@ -2247,6 +2255,24 @@ assert_equal '[[1, 2]]', %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_yjit.rb#L2255 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:) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index 227f1be7e79..c0230f74190 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -506,6 +506,17 @@ class TestYJIT < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_yjit.rb#L506 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) diff --git a/yjit_codegen.c b/yjit_codegen.c index 38b830a0974..3782d8eea37 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -3440,25 +3440,6 @@ gen_return_branch(codeblock_t *cb, uint8_t *target0, uint8_t *target1, uint8_t s https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L3440 } } -// 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) @@ -3482,6 +3463,20 @@ rb_leaf_builtin_function(const rb_iseq_t *iseq) https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L3463 return (const struct rb_builtin_function *)iseq->body->iseq_encoded[1]; } +static bool +iseq_supported_args_p(const rb_iseq_t *iseq) +{ + // supports has_opt + // supports has_kw + // supports accepts_no_kwarg + bool takes_block = iseq->body->param.flags.has_block; + return (!takes_block || iseq->body->local_iseq == iseq) && + iseq->body->param.flags.has_rest == false && + iseq->body->param.flags.has_post == false && + iseq->body->param.flags.has_kwrest == false; +} + + static codegen_status_t gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const rb_callable_method_entry_t *cme, rb_iseq_t *block, int32_t argc) { @@ -3492,7 +3487,7 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L3487 // 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; + bool doing_kw_call = iseq->body->param.flags.has_kw; if (vm_ci_flag(ci) & VM_CALL_TAILCALL) { // We can't handle tailcalls @@ -3500,66 +3495,69 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r https://github.com/ruby/ruby/blob/trunk/yjit_codegen.c#L3495 return YJIT_CANT_COMPILE; } + if (!iseq_supported_args_p(iseq)) { + 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 ((vm_ci_flag(ci) & VM_CALL_KWARG) && !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 ((vm_ci_flag(ci) & VM_CALL_KWARG) && iseq->body->param.flags.accepts_no_kwarg) { + GEN_COUNTER_INC(cb, send_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + // Arity handling and optional parameter setup int num_params = iseq->body->param.size; - uint32_t start_pc_offset = 0; - 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; - } + // Remove blockarg if sent via ENV + if (iseq->body->param.flags.has_block && iseq->body->local_iseq == iseq) { + num_params--; + } - num_params = iseq->body->param.lead_num; + uint32_t start_pc_offset = 0; - if (num_params != argc) { - GEN_COUNTER_INC(cb, send_iseq_arity_error); - return YJIT_CANT_COMPILE; - } - } - 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) { - GEN_COUNTER_INC(cb, send_iseq_complex_callee); - return YJIT_CANT_COMPILE; - } + const int required_num = iseq->body->param.lead_num; - // 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)); + // 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; - 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 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 (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 (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/