From 66680ce59817d77be85c1d2b3848c8b012d92bd0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 21 Apr 2021 13:01:03 -0700 Subject: [PATCH 1/5] Improve set instance variable This commit improves the set ivar implementation. --- bootstraptest/test_yjit.rb | 85 ++++++++++++++++++ yjit_codegen.c | 176 +++++++++++++++++++++++++------------ yjit_iface.h | 7 ++ 3 files changed, 212 insertions(+), 56 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 2da9086a60202e..5c7e74635a4e80 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -239,6 +239,91 @@ def itself end } +# test setinstancevariable on extended objects +assert_equal '1', %q{ + class Extended + attr_reader :one + + def write_many + @a = 1 + @b = 2 + @c = 3 + @d = 4 + @one = 1 + end + end + + foo = Extended.new + foo.write_many + foo.write_many + foo.write_many +} + +# test setinstancevariable on embedded objects +assert_equal '1', %q{ + class Embedded + attr_reader :one + + def write_one + @one = 1 + end + end + + foo = Embedded.new + foo.write_one + foo.write_one + foo.write_one +} + +# test setinstancevariable after extension +assert_equal '[10, 11, 12, 13, 1]', %q{ + class WillExtend + attr_reader :one + + def make_extended + @foo1 = 10 + @foo2 = 11 + @foo3 = 12 + @foo4 = 13 + end + + def write_one + @one = 1 + end + + def read_all + [@foo1, @foo2, @foo3, @foo4, @one] + end + end + + foo = WillExtend.new + foo.write_one + foo.write_one + foo.make_extended + foo.write_one + foo.read_all +} + +# test setinstancevariable on frozen object +assert_equal 'object was not modified', %q{ + class WillFreeze + def write + @ivar = 1 + end + end + + wf = WillFreeze.new + wf.write + wf.write + wf.freeze + + begin + wf.write + rescue FrozenError + "object was not modified" + end +} + # Test getinstancevariable and inline caches assert_equal '6', %q{ class Foo diff --git a/yjit_codegen.c b/yjit_codegen.c index 397f0245ade006..53c6d27c92c9ac 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -744,6 +744,112 @@ enum { OSWB_MAX_DEPTH = 5, // up to 5 different classes }; +// Codegen for setting an instance variable. +// Preconditions: +// - receiver is in REG0 +// - receiver has the same class as CLASS_OF(comptime_receiver) +// - no stack push or pops to ctx since the entry to the codegen of the instruction being compiled +static codegen_status_t +gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE comptime_receiver, ID ivar_name, insn_opnd_t reg0_opnd, uint8_t *side_exit) +{ + VALUE comptime_val_klass = CLASS_OF(comptime_receiver); + const ctx_t starting_context = *ctx; // make a copy for use with jit_chain_guard + + // If the class uses the default allocator, instances should all be T_OBJECT + // NOTE: This assumes nobody changes the allocator of the class after allocation. + // Eventually, we can encode whether an object is T_OBJECT or not + // inside object shapes. + if (rb_get_alloc_func(comptime_val_klass) != rb_class_allocate_instance) { + GEN_COUNTER_INC(cb, setivar_not_object); + return YJIT_CANT_COMPILE; + } + RUBY_ASSERT(BUILTIN_TYPE(comptime_receiver) == T_OBJECT); // because we checked the allocator + + // ID for the name of the ivar + ID id = ivar_name; + struct rb_iv_index_tbl_entry *ent; + struct st_table *iv_index_tbl = ROBJECT_IV_INDEX_TBL(comptime_receiver); + + // Bail if this is a heap object, because this needs a write barrier + ADD_COMMENT(cb, "guard value is immediate"); + test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); + jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_val_heapobject)); + + // Lookup index for the ivar the instruction loads + if (iv_index_tbl && rb_iv_index_tbl_lookup(iv_index_tbl, id, &ent)) { + uint32_t ivar_index = ent->index; + + x86opnd_t val_to_write = ctx_stack_pop(ctx, 1); + mov(cb, REG1, val_to_write); + + x86opnd_t flags_opnd = member_opnd(REG0, struct RBasic, flags); + + // Bail if this object is frozen + ADD_COMMENT(cb, "guard self is not frozen"); + test(cb, flags_opnd, imm_opnd(RUBY_FL_FREEZE)); + jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_frozen)); + + // Pop receiver if it's on the temp stack + if (!reg0_opnd.is_self) { + (void)ctx_stack_pop(ctx, 1); + } + + // Compile time self is embedded and the ivar index lands within the object + if (RB_FL_TEST_RAW(comptime_receiver, ROBJECT_EMBED) && ivar_index < ROBJECT_EMBED_LEN_MAX) { + // See ROBJECT_IVPTR() from include/ruby/internal/core/robject.h + + // Guard that self is embedded + // TODO: BT and JC is shorter + ADD_COMMENT(cb, "guard embedded setivar"); + test(cb, flags_opnd, imm_opnd(ROBJECT_EMBED)); + jit_chain_guard(JCC_JZ, jit, &starting_context, max_chain_depth, side_exit); + + // Load the variable + x86opnd_t ivar_opnd = mem_opnd(64, REG0, offsetof(struct RObject, as.ary) + ivar_index * SIZEOF_VALUE); + + mov(cb, ivar_opnd, REG1); + + // Push the ivar on the stack + // For attr_writer we'll need to push the value on the stack + //x86opnd_t out_opnd = ctx_stack_push(ctx, TYPE_UNKNOWN); + } + else { + // Compile time value is *not* embeded. + + // Guard that value is *not* embedded + // See ROBJECT_IVPTR() from include/ruby/internal/core/robject.h + ADD_COMMENT(cb, "guard extended setivar"); + x86opnd_t flags_opnd = member_opnd(REG0, struct RBasic, flags); + test(cb, flags_opnd, imm_opnd(ROBJECT_EMBED)); + jit_chain_guard(JCC_JNZ, jit, &starting_context, max_chain_depth, side_exit); + + // check that the extended table is big enough + if (ivar_index >= ROBJECT_EMBED_LEN_MAX + 1) { + // Check that the slot is inside the extended table (num_slots > index) + x86opnd_t num_slots = mem_opnd(32, REG0, offsetof(struct RObject, as.heap.numiv)); + cmp(cb, num_slots, imm_opnd(ivar_index)); + jle_ptr(cb, COUNTED_EXIT(side_exit, setivar_idx_out_of_range)); + } + + // Get a pointer to the extended table + x86opnd_t tbl_opnd = mem_opnd(64, REG0, offsetof(struct RObject, as.heap.ivptr)); + mov(cb, REG0, tbl_opnd); + + // Write the ivar to the extended table + x86opnd_t ivar_opnd = mem_opnd(64, REG0, sizeof(VALUE) * ivar_index); + mov(cb, REG1, val_to_write); + mov(cb, ivar_opnd, REG1); + } + + // Jump to next instruction. This allows guard chains to share the same successor. + jit_jump_to_next_insn(jit, ctx); + return YJIT_END_BLOCK; + } + + GEN_COUNTER_INC(cb, setivar_name_not_mapped); + return YJIT_CANT_COMPILE; +} + // Codegen for getting an instance variable. // Preconditions: // - receiver is in REG0 @@ -866,7 +972,7 @@ gen_getinstancevariable(jitstate_t *jit, ctx_t *ctx) // Guard that the receiver has the same class as the one from compile time. mov(cb, REG0, member_opnd(REG_CFP, rb_control_frame_t, self)); - guard_self_is_heap(cb, REG0, side_exit, ctx); + guard_self_is_heap(cb, REG0, COUNTED_EXIT(side_exit, getivar_se_self_not_heap), ctx); jit_guard_known_klass(jit, ctx, comptime_val_klass, OPND_SELF, GETIVAR_MAX_DEPTH, side_exit); @@ -876,69 +982,27 @@ gen_getinstancevariable(jitstate_t *jit, ctx_t *ctx) static codegen_status_t gen_setinstancevariable(jitstate_t* jit, ctx_t* ctx) { - IVC ic = (IVC)jit_get_arg(jit, 1); - - // Check that the inline cache has been set, slot index is known - if (!ic->entry) { - return YJIT_CANT_COMPILE; + // Defer compilation so we can specialize on a runtime `self` + if (!jit_at_current_insn(jit)) { + defer_compilation(jit->block, jit->insn_idx, ctx); + return YJIT_END_BLOCK; } - // If the class uses the default allocator, instances should all be T_OBJECT - // NOTE: This assumes nobody changes the allocator of the class after allocation. - // Eventually, we can encode whether an object is T_OBJECT or not - // inside object shapes. - if (rb_get_alloc_func(ic->entry->class_value) != rb_class_allocate_instance) { - return YJIT_CANT_COMPILE; - } + ID ivar_name = (ID)jit_get_arg(jit, 0); - uint32_t ivar_index = ic->entry->index; + VALUE comptime_val = jit_peek_at_self(jit, ctx); + VALUE comptime_val_klass = CLASS_OF(comptime_val); - // Create a size-exit to fall back to the interpreter - uint8_t* side_exit = yjit_side_exit(jit, ctx); + // Generate a side exit + uint8_t *side_exit = yjit_side_exit(jit, ctx); - // Load self from CFP + // Guard that the receiver has the same class as the one from compile time. mov(cb, REG0, member_opnd(REG_CFP, rb_control_frame_t, self)); + guard_self_is_heap(cb, REG0, COUNTED_EXIT(side_exit, setivar_se_self_not_heap), ctx); - guard_self_is_heap(cb, REG0, side_exit, ctx); - - // Bail if receiver class is different from compiled time call cache class - x86opnd_t klass_opnd = mem_opnd(64, REG0, offsetof(struct RBasic, klass)); - mov(cb, REG1, klass_opnd); - x86opnd_t serial_opnd = mem_opnd(64, REG1, offsetof(struct RClass, class_serial)); - cmp(cb, serial_opnd, imm_opnd(ic->entry->class_serial)); - jne_ptr(cb, side_exit); - - // Bail if the ivars are not on the extended table - // See ROBJECT_IVPTR() from include/ruby/internal/core/robject.h - x86opnd_t flags_opnd = member_opnd(REG0, struct RBasic, flags); - test(cb, flags_opnd, imm_opnd(ROBJECT_EMBED)); - jnz_ptr(cb, side_exit); - - // If we can't guarantee that the extended table is big enoughg - if (ivar_index >= ROBJECT_EMBED_LEN_MAX + 1) { - // Check that the slot is inside the extended table (num_slots > index) - x86opnd_t num_slots = mem_opnd(32, REG0, offsetof(struct RObject, as.heap.numiv)); - cmp(cb, num_slots, imm_opnd(ivar_index)); - jle_ptr(cb, side_exit); - } - - // Get a pointer to the extended table - x86opnd_t tbl_opnd = mem_opnd(64, REG0, offsetof(struct RObject, as.heap.ivptr)); - mov(cb, REG0, tbl_opnd); - - // Pop the value to write from the stack - x86opnd_t stack_top = ctx_stack_pop(ctx, 1); - mov(cb, REG1, stack_top); - - // Bail if this is a heap object, because this needs a write barrier - test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); - jz_ptr(cb, side_exit); - - // Write the ivar to the extended table - x86opnd_t ivar_opnd = mem_opnd(64, REG0, sizeof(VALUE) * ivar_index); - mov(cb, ivar_opnd, REG1); + jit_guard_known_klass(jit, ctx, comptime_val_klass, OPND_SELF, GETIVAR_MAX_DEPTH, side_exit); - return YJIT_KEEP_COMPILING; + return gen_set_ivar(jit, ctx, GETIVAR_MAX_DEPTH, comptime_val, ivar_name, OPND_SELF, side_exit); } static void diff --git a/yjit_iface.h b/yjit_iface.h index cc8d3f7012bbf0..eabaff19554f78 100644 --- a/yjit_iface.h +++ b/yjit_iface.h @@ -57,6 +57,13 @@ YJIT_DECLARE_COUNTERS( getivar_name_not_mapped, getivar_not_object, + setivar_se_self_not_heap, + setivar_idx_out_of_range, + setivar_val_heapobject, + setivar_name_not_mapped, + setivar_not_object, + setivar_frozen, + oaref_argc_not_one, oaref_arg_not_fixnum, From 565473a5d4a29aecb7b7edb0fda6406b40e0a69a Mon Sep 17 00:00:00 2001 From: Maxime Chevalier-Boisvert Date: Wed, 21 Apr 2021 17:16:44 -0400 Subject: [PATCH 2/5] Add setivar exit reasons to --yjit-stats --- yjit.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/yjit.rb b/yjit.rb index a0ff8d11d227e7..8203051b81dbf3 100644 --- a/yjit.rb +++ b/yjit.rb @@ -76,17 +76,16 @@ class << self # Format and print out counters def _print_stats counters = runtime_stats - return unless counters $stderr.puts("***YJIT: Printing runtime counters from yjit.rb***") - - $stderr.puts "Number of bindings allocated: %d\n" % counters[:binding_allocations] - $stderr.puts "Number of locals modified through binding: %d\n" % counters[:binding_set] + $stderr.puts("Number of bindings allocated: %d\n" % counters[:binding_allocations]) + $stderr.puts("Number of locals modified through binding: %d\n" % counters[:binding_set]) print_counters(counters, prefix: 'oswb_', prompt: 'opt_send_without_block exit reasons: ') print_counters(counters, prefix: 'leave_', prompt: 'leave exit reasons: ') print_counters(counters, prefix: 'getivar_', prompt: 'getinstancevariable exit reasons:') + print_counters(counters, prefix: 'setivar_', prompt: 'setinstancevariable exit reasons:') print_counters(counters, prefix: 'oaref_', prompt: 'opt_aref exit reasons: ') end From 61723b73a9a8b09b6f434cfb50cdc0042d49721d Mon Sep 17 00:00:00 2001 From: Maxime Chevalier-Boisvert Date: Wed, 21 Apr 2021 17:24:49 -0400 Subject: [PATCH 3/5] Fix frozen check (use jnz) and move heap object check. --- yjit_codegen.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/yjit_codegen.c b/yjit_codegen.c index 53c6d27c92c9ac..bb16e1b37b2f05 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -770,11 +770,6 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt struct rb_iv_index_tbl_entry *ent; struct st_table *iv_index_tbl = ROBJECT_IV_INDEX_TBL(comptime_receiver); - // Bail if this is a heap object, because this needs a write barrier - ADD_COMMENT(cb, "guard value is immediate"); - test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); - jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_val_heapobject)); - // Lookup index for the ivar the instruction loads if (iv_index_tbl && rb_iv_index_tbl_lookup(iv_index_tbl, id, &ent)) { uint32_t ivar_index = ent->index; @@ -782,12 +777,16 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt x86opnd_t val_to_write = ctx_stack_pop(ctx, 1); mov(cb, REG1, val_to_write); - x86opnd_t flags_opnd = member_opnd(REG0, struct RBasic, flags); + // Bail if the value to write is a heap object, because this needs a write barrier + ADD_COMMENT(cb, "guard value is immediate"); + test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); + jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_val_heapobject)); // Bail if this object is frozen ADD_COMMENT(cb, "guard self is not frozen"); + x86opnd_t flags_opnd = member_opnd(REG0, struct RBasic, flags); test(cb, flags_opnd, imm_opnd(RUBY_FL_FREEZE)); - jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_frozen)); + jnz_ptr(cb, COUNTED_EXIT(side_exit, setivar_frozen)); // Pop receiver if it's on the temp stack if (!reg0_opnd.is_self) { From a79ed42468bd3c0ce94c1095df4a4bb4e2e7d692 Mon Sep 17 00:00:00 2001 From: Maxime Chevalier-Boisvert Date: Wed, 21 Apr 2021 23:36:53 -0400 Subject: [PATCH 4/5] Remove redundant mov --- yjit_codegen.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/yjit_codegen.c b/yjit_codegen.c index bb16e1b37b2f05..d28e7806890dc0 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -803,9 +803,8 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt test(cb, flags_opnd, imm_opnd(ROBJECT_EMBED)); jit_chain_guard(JCC_JZ, jit, &starting_context, max_chain_depth, side_exit); - // Load the variable + // Store the ivar on the object x86opnd_t ivar_opnd = mem_opnd(64, REG0, offsetof(struct RObject, as.ary) + ivar_index * SIZEOF_VALUE); - mov(cb, ivar_opnd, REG1); // Push the ivar on the stack @@ -825,6 +824,7 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt // check that the extended table is big enough if (ivar_index >= ROBJECT_EMBED_LEN_MAX + 1) { // Check that the slot is inside the extended table (num_slots > index) + ADD_COMMENT(cb, "check index in extended table"); x86opnd_t num_slots = mem_opnd(32, REG0, offsetof(struct RObject, as.heap.numiv)); cmp(cb, num_slots, imm_opnd(ivar_index)); jle_ptr(cb, COUNTED_EXIT(side_exit, setivar_idx_out_of_range)); @@ -836,7 +836,6 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt // Write the ivar to the extended table x86opnd_t ivar_opnd = mem_opnd(64, REG0, sizeof(VALUE) * ivar_index); - mov(cb, REG1, val_to_write); mov(cb, ivar_opnd, REG1); } From 8b558c5c8f7052021a2995d19b9ec2bbe947e654 Mon Sep 17 00:00:00 2001 From: Maxime Chevalier-Boisvert Date: Thu, 22 Apr 2021 10:10:15 -0400 Subject: [PATCH 5/5] Don't check if value is immediate if context has type info --- yjit_codegen.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/yjit_codegen.c b/yjit_codegen.c index d28e7806890dc0..8402d1eeed15c4 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -774,13 +774,20 @@ gen_set_ivar(jitstate_t *jit, ctx_t *ctx, const int max_chain_depth, VALUE compt if (iv_index_tbl && rb_iv_index_tbl_lookup(iv_index_tbl, id, &ent)) { uint32_t ivar_index = ent->index; - x86opnd_t val_to_write = ctx_stack_pop(ctx, 1); + val_type_t val_type = ctx_get_opnd_type(ctx, OPND_STACK(0)); + x86opnd_t val_to_write = ctx_stack_opnd(ctx, 0); mov(cb, REG1, val_to_write); // Bail if the value to write is a heap object, because this needs a write barrier - ADD_COMMENT(cb, "guard value is immediate"); - test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); - jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_val_heapobject)); + if (!val_type.is_imm) { + ADD_COMMENT(cb, "guard value is immediate"); + test(cb, REG1, imm_opnd(RUBY_IMMEDIATE_MASK)); + jz_ptr(cb, COUNTED_EXIT(side_exit, setivar_val_heapobject)); + ctx_set_opnd_type(ctx, OPND_STACK(0), TYPE_IMM); + } + + // Pop the value to write + ctx_stack_pop(ctx, 1); // Bail if this object is frozen ADD_COMMENT(cb, "guard self is not frozen");