Skip to content
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.

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

YJIT: Pass nil to anonymous kwrest when empty #9972

Merged
merged 1 commit into from Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions yjit.c
Expand Up @@ -659,6 +659,12 @@ rb_get_iseq_flags_has_kwrest(const rb_iseq_t *iseq)
return iseq->body->param.flags.has_kwrest;
}

bool
rb_get_iseq_flags_anon_kwrest(const rb_iseq_t *iseq)
{
return iseq->body->param.flags.anon_kwrest;
}

bool
rb_get_iseq_flags_has_rest(const rb_iseq_t *iseq)
{
Expand Down
1 change: 1 addition & 0 deletions yjit/bindgen/src/main.rs
Expand Up @@ -421,6 +421,7 @@ fn main() {
.allowlist_function("rb_get_iseq_flags_has_rest")
.allowlist_function("rb_get_iseq_flags_has_post")
.allowlist_function("rb_get_iseq_flags_has_kwrest")
.allowlist_function("rb_get_iseq_flags_anon_kwrest")
.allowlist_function("rb_get_iseq_flags_has_block")
.allowlist_function("rb_get_iseq_flags_ambiguous_param0")
.allowlist_function("rb_get_iseq_flags_accepts_no_kwarg")
Expand Down
43 changes: 33 additions & 10 deletions yjit/src/codegen.rs
Expand Up @@ -1527,6 +1527,19 @@ fn gen_splatkw(
// If a compile-time hash operand is T_HASH, just guard that it's T_HASH.
let hash_opnd = asm.stack_opnd(1);
guard_object_is_hash(asm, hash_opnd, hash_opnd.into(), Counter::splatkw_not_hash);
} else if comptime_hash.nil_p() {
// Speculate we'll see nil if compile-time hash operand is nil
let hash_opnd = asm.stack_opnd(1);
let hash_opnd_type = asm.ctx.get_opnd_type(hash_opnd.into());

if hash_opnd_type != Type::Nil {
asm.cmp(hash_opnd, Qnil.into());
asm.jne(Target::side_exit(Counter::splatkw_not_nil));

if Type::Nil.diff(hash_opnd_type) != TypeDiff::Incompatible {
asm.ctx.upgrade_opnd_type(hash_opnd.into(), Type::Nil);
}
}
} else {
// Otherwise, call #to_hash on the operand if it's not nil.

Expand Down Expand Up @@ -7288,6 +7301,7 @@ fn gen_iseq_kw_call(
unsafe { get_cikw_keyword_len(ci_kwarg) }
};
let caller_keyword_len: usize = caller_keyword_len_i32.try_into().unwrap();
let anon_kwrest = unsafe { rb_get_iseq_flags_anon_kwrest(iseq) };

// This struct represents the metadata about the callee-specified
// keyword parameters.
Expand Down Expand Up @@ -7368,16 +7382,25 @@ fn gen_iseq_kw_call(
}
}

// Save PC and SP before allocating
jit_save_pc(jit, asm);
gen_save_sp(asm);
let (kwrest, kwrest_type) = if rest_mask == 0 && anon_kwrest {
// In case the kwrest hash should be empty and is anonymous in the callee,
// we can pass nil instead of allocating. Anonymous kwrest can only be
// delegated, and nil is the same as an empty hash when delegating.
(Qnil.into(), Type::Nil)
} else {
// Save PC and SP before allocating
jit_save_pc(jit, asm);
gen_save_sp(asm);

// Build the kwrest hash. `struct rb_callinfo_kwarg` is malloc'd, so no GC concerns.
let kwargs_start = asm.lea(asm.ctx.sp_opnd(-caller_keyword_len_i32 * SIZEOF_VALUE_I32));
let hash = asm.ccall(
build_kw_rest as _,
vec![rest_mask.into(), kwargs_start, Opnd::const_ptr(ci_kwarg.cast())]
);
(hash, Type::THash)
};

// Build the kwrest hash. `struct rb_callinfo_kwarg` is malloc'd, so no GC concerns.
let kwargs_start = asm.lea(asm.ctx.sp_opnd(-caller_keyword_len_i32 * SIZEOF_VALUE_I32));
let kwrest = asm.ccall(
build_kw_rest as _,
vec![rest_mask.into(), kwargs_start, Opnd::const_ptr(ci_kwarg.cast())]
);
// The kwrest parameter sits after `unspecified_bits` if the callee specifies any
// keywords.
let stack_kwrest_idx = kwargs_stack_base - callee_kw_count_i32 - i32::from(callee_kw_count > 0);
Expand All @@ -7399,7 +7422,7 @@ fn gen_iseq_kw_call(
asm.ctx.dealloc_temp_reg(stack_kwrest.stack_idx());
asm.mov(stack_kwrest, kwrest);
if stack_kwrest_idx >= 0 {
asm.ctx.set_opnd_mapping(stack_kwrest.into(), TempMapping::map_to_stack(Type::THash));
asm.ctx.set_opnd_mapping(stack_kwrest.into(), TempMapping::map_to_stack(kwrest_type));
}
}

Expand Down
1 change: 1 addition & 0 deletions yjit/src/cruby_bindings.inc.rs
Expand Up @@ -1149,6 +1149,7 @@ extern "C" {
pub fn rb_get_iseq_flags_has_kw(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_has_post(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_has_kwrest(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_anon_kwrest(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_has_rest(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_ruby2_keywords(iseq: *const rb_iseq_t) -> bool;
pub fn rb_get_iseq_flags_has_block(iseq: *const rb_iseq_t) -> bool;
Expand Down
1 change: 1 addition & 0 deletions yjit/src/stats.rs
Expand Up @@ -544,6 +544,7 @@ make_counters! {
objtostring_not_string,

splatkw_not_hash,
splatkw_not_nil,

binding_allocations,
binding_set,
Expand Down