From 4943ee13032b1ccd3dc710d0f9be4c083095139f Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 16 Dec 2021 10:19:24 -0800 Subject: [PATCH 1/3] 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 0b2b78ca4a9737..501f00d5b3437b 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -2226,6 +2226,14 @@ 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 '[[1, 2]]', %q{ def kwargs(left:, right:) [left, right] @@ -2247,6 +2255,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:) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index 227f1be7e796b9..c0230f74190a02 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -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) diff --git a/yjit_codegen.c b/yjit_codegen.c index 38b830a0974da6..3782d8eea37d0b 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 } } -// 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) 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 // 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 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 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; @@ -3572,13 +3570,8 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } + // Check that the kwargs being passed are valid 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; - } - // This is the list of keyword arguments that the callee specified // in its initial declaration. const ID *callee_kwargs = keyword->table; @@ -3612,31 +3605,12 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } } - } - 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); + } else if (keyword->required_num != 0) { + // No keywords provided so if any are required this is a mismatch + 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; From 853bd647fa7913f92b6b41d63b79f3839cfa967b Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 16 Dec 2021 15:08:07 -0800 Subject: [PATCH 2/3] YJIT: Fix check for required kwargs Previously, YJIT would not check that all the required keywords were specified in the case that there were optional arguments specified. In this case YJIT would incorrectly call the method with invalid arguments. --- bootstraptest/test_yjit.rb | 8 ++++++++ yjit_codegen.c | 13 +++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 501f00d5b3437b..05947c48ed21f2 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -2234,6 +2234,14 @@ def kwargs(value:) 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] diff --git a/yjit_codegen.c b/yjit_codegen.c index 3782d8eea37d0b..b18ec44c7a3c4f 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -3562,6 +3562,8 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r // 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 @@ -3604,9 +3606,16 @@ 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 (keyword->required_num != 0) { - // No keywords provided so if any are required this is a mismatch + } + + 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; } From c3e5ee256ce66168f32456d3c9f977c186956afd Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Fri, 17 Dec 2021 11:44:55 -0500 Subject: [PATCH 3/3] YJIT: Remove double check for block arg handling Inline and remove iseq_supported_args_p(iseq) to remove a potentially dangerous double check on `iseq->body->param.flags.has_block` and `iseq->body->local_iseq == iseq`. Double checking should be fine at the moment as there should be no case where we perform a call to an iseq that takes a block but `local_iseq != iseq`, but such situation might be possible when we add support for calling into BMETHODs, for example. Inlining also has the benefit of mirroring the interpreter's code for blockarg setup in `setup_parameters_complex()`, making checking for parity easier. Extract `vm_ci_flag(ci) & VM_CALL_KWARG` into a const local for brevity. Constify `doing_kw_call` because we can. --- yjit_codegen.c | 51 ++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/yjit_codegen.c b/yjit_codegen.c index b18ec44c7a3c4f..eebb129df8ad1e 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -3463,20 +3463,6 @@ rb_leaf_builtin_function(const rb_iseq_t *iseq) 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) { @@ -3487,7 +3473,8 @@ 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 = iseq->body->param.flags.has_kw; + 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 @@ -3495,7 +3482,11 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } - if (!iseq_supported_args_p(iseq)) { + // 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; } @@ -3503,24 +3494,35 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r // 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) { + 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 ((vm_ci_flag(ci) & VM_CALL_KWARG) && iseq->body->param.flags.accepts_no_kwarg) { + if (supplying_kws && 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 + // For computing number of locals to setup for the callee int num_params = iseq->body->param.size; - // Remove blockarg if sent via ENV - if (iseq->body->param.flags.has_block && iseq->body->local_iseq == iseq) { - num_params--; + // 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 { + // 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; + } } uint32_t start_pc_offset = 0; @@ -3532,6 +3534,7 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r 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; @@ -3573,7 +3576,7 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r } // Check that the kwargs being passed are valid - if (vm_ci_flag(ci) & VM_CALL_KWARG) { + if (supplying_kws) { // This is the list of keyword arguments that the callee specified // in its initial declaration. const ID *callee_kwargs = keyword->table; @@ -3699,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;