ruby-changes:51651
From: k0kubun <ko1@a...>
Date: Fri, 6 Jul 2018 00:56:54 +0900 (JST)
Subject: [ruby-changes:51651] k0kubun:r63863 (trunk): revert r62655 for r63763
k0kubun 2018-07-06 00:56:48 +0900 (Fri, 06 Jul 2018) New Revision: 63863 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=63863 Log: revert r62655 for r63763 r63655 was tightly coupled to handle_frames and some assumptions seems to have been broken by r63763. To partially resolve Bug#14892, this reverts the optimization for now. I want to make MJIT CI happy first and then I'll probably retry r63655 by partially reverting r63763 for sp changes. The skipped test is not fixed yet. Modified files: trunk/insns.def trunk/mjit_compile.c trunk/test/ruby/test_jit.rb trunk/tool/ruby_vm/views/_mjit_compile_insn.erb trunk/tool/ruby_vm/views/_mjit_compile_insn_body.erb trunk/tool/ruby_vm/views/_mjit_compile_pc_and_sp.erb trunk/tool/ruby_vm/views/_mjit_compile_send.erb trunk/tool/ruby_vm/views/mjit_compile.inc.erb Index: mjit_compile.c =================================================================== --- mjit_compile.c (revision 63862) +++ mjit_compile.c (revision 63863) @@ -24,9 +24,6 @@ https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L24 struct compile_status { int success; /* has TRUE if compilation has had no issue */ int *stack_size_for_pos; /* stack_size_for_pos[pos] has stack size for the position (otherwise -1) */ - /* If TRUE, JIT-ed code will use local variables to store pushed values instead of - using VM's stack and moving stack pointer. */ - int local_stack_p; }; /* Storage to keep data which is consistent in each conditional branch. @@ -175,13 +172,7 @@ compile_insns(FILE *f, const struct rb_i https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L172 static void compile_cancel_handler(FILE *f, const struct rb_iseq_constant_body *body, struct compile_status *status) { - unsigned int i; fprintf(f, "\ncancel:\n"); - if (status->local_stack_p) { - for (i = 0; i < body->stack_max; i++) { - fprintf(f, " *((VALUE *)reg_cfp->bp + %d) = stack[%d];\n", i + 1, i); - } - } fprintf(f, " return Qundef;\n"); } @@ -191,7 +182,6 @@ mjit_compile(FILE *f, const struct rb_is https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L182 { struct compile_status status; status.success = TRUE; - status.local_stack_p = !body->catch_except_p; status.stack_size_for_pos = ALLOC_N(int, body->iseq_size); memset(status.stack_size_for_pos, NOT_COMPILED_STACK_SIZE, sizeof(int) * body->iseq_size); @@ -205,12 +195,7 @@ mjit_compile(FILE *f, const struct rb_is https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L195 fprintf(f, "__declspec(dllexport)\n"); #endif fprintf(f, "VALUE\n%s(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp)\n{\n", funcname); - if (status.local_stack_p) { - fprintf(f, " VALUE stack[%d];\n", body->stack_max); - } - else { - fprintf(f, " VALUE *stack = reg_cfp->sp;\n"); - } + fprintf(f, " VALUE *stack = reg_cfp->sp;\n"); fprintf(f, " static const VALUE *const original_body_iseq = (VALUE *)0x%"PRIxVALUE";\n", (VALUE)body->iseq_encoded); Index: insns.def =================================================================== --- insns.def (revision 63862) +++ insns.def (revision 63863) @@ -989,9 +989,9 @@ opt_plus https://github.com/ruby/ruby/blob/trunk/insns.def#L989 val = vm_opt_plus(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1008,9 +1008,9 @@ opt_minus https://github.com/ruby/ruby/blob/trunk/insns.def#L1008 val = vm_opt_minus(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1027,9 +1027,9 @@ opt_mult https://github.com/ruby/ruby/blob/trunk/insns.def#L1027 val = vm_opt_mult(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1046,9 +1046,9 @@ opt_div https://github.com/ruby/ruby/blob/trunk/insns.def#L1046 val = vm_opt_div(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1065,9 +1065,9 @@ opt_mod https://github.com/ruby/ruby/blob/trunk/insns.def#L1065 val = vm_opt_mod(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1084,9 +1084,9 @@ opt_eq https://github.com/ruby/ruby/blob/trunk/insns.def#L1084 val = opt_eq_func(recv, obj, ci, cc); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1103,9 +1103,9 @@ opt_neq https://github.com/ruby/ruby/blob/trunk/insns.def#L1103 val = vm_opt_neq(ci, cc, ci_eq, cc_eq, recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1122,9 +1122,9 @@ opt_lt https://github.com/ruby/ruby/blob/trunk/insns.def#L1122 val = vm_opt_lt(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1141,9 +1141,9 @@ opt_le https://github.com/ruby/ruby/blob/trunk/insns.def#L1141 val = vm_opt_le(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1160,9 +1160,9 @@ opt_gt https://github.com/ruby/ruby/blob/trunk/insns.def#L1160 val = vm_opt_gt(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1179,9 +1179,9 @@ opt_ge https://github.com/ruby/ruby/blob/trunk/insns.def#L1179 val = vm_opt_ge(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1198,9 +1198,9 @@ opt_ltlt https://github.com/ruby/ruby/blob/trunk/insns.def#L1198 val = vm_opt_ltlt(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1217,9 +1217,9 @@ opt_aref https://github.com/ruby/ruby/blob/trunk/insns.def#L1217 val = vm_opt_aref(recv, obj); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1236,10 +1236,10 @@ opt_aset https://github.com/ruby/ruby/blob/trunk/insns.def#L1236 val = vm_opt_aset(recv, obj, set); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(obj); PUSH(set); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1259,10 +1259,10 @@ opt_aset_with https://github.com/ruby/ruby/blob/trunk/insns.def#L1259 val = tmp; } else { -#ifndef MJIT_HEADER PUSH(recv); PUSH(rb_str_resurrect(key)); PUSH(val); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1279,9 +1279,9 @@ opt_aref_with https://github.com/ruby/ruby/blob/trunk/insns.def#L1279 val = vm_opt_aref_with(recv, key); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); PUSH(rb_str_resurrect(key)); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1298,8 +1298,8 @@ opt_length https://github.com/ruby/ruby/blob/trunk/insns.def#L1298 val = vm_opt_length(recv, BOP_LENGTH); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1316,8 +1316,8 @@ opt_size https://github.com/ruby/ruby/blob/trunk/insns.def#L1316 val = vm_opt_length(recv, BOP_SIZE); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1334,8 +1334,8 @@ opt_empty_p https://github.com/ruby/ruby/blob/trunk/insns.def#L1334 val = vm_opt_empty_p(recv); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1352,8 +1352,8 @@ opt_succ https://github.com/ruby/ruby/blob/trunk/insns.def#L1352 val = vm_opt_succ(recv); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1370,8 +1370,8 @@ opt_not https://github.com/ruby/ruby/blob/trunk/insns.def#L1370 val = vm_opt_not(ci, cc, recv); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(recv); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); @@ -1398,9 +1398,9 @@ opt_regexpmatch2 https://github.com/ruby/ruby/blob/trunk/insns.def#L1398 val = vm_opt_regexpmatch2(obj2, obj1); if (val == Qundef) { -#ifndef MJIT_HEADER PUSH(obj2); PUSH(obj1); +#ifndef MJIT_HEADER ADD_PC(-WIDTH_OF_opt_send_without_block); #endif DISPATCH_ORIGINAL_INSN(opt_send_without_block); Index: tool/ruby_vm/views/_mjit_compile_send.erb =================================================================== --- tool/ruby_vm/views/_mjit_compile_send.erb (revision 63862) +++ tool/ruby_vm/views/_mjit_compile_send.erb (revision 63863) @@ -24,10 +24,6 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_send.erb#L24 int param_size = iseq->body->param.size; /* TODO: check calling->argc for argument_arity_error */ fprintf(f, "{\n"); -% # JIT: Declare stack_size to be used in some macro of _mjit_compile_insn_body.erb - if (status->local_stack_p) { - fprintf(f, " MAYBE_UNUSED(unsigned int) stack_size = %u;\n", b->stack_size); - } % # JIT: Invalidate call cache if it requires vm_search_method. This allows to inline some of following things. <%= render 'mjit_compile_send_guard' -%> Index: tool/ruby_vm/views/mjit_compile.inc.erb =================================================================== --- tool/ruby_vm/views/mjit_compile.inc.erb (revision 63862) +++ tool/ruby_vm/views/mjit_compile.inc.erb (revision 63863) @@ -31,13 +31,13 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/mjit_compile.inc.erb#L31 % # reg_cfp: the second argument of _mjitXXX % # GET_CFP(): refers to `reg_cfp` % # GET_EP(): refers to `reg_cfp->ep` -% # GET_SP(): refers to `reg_cfp->sp`, or `(stack + stack_size)` if local_stack_p +% # GET_SP(): refers to `reg_cfp->sp` % # GET_SELF(): refers to `reg_cfp->self` % # GET_LEP(): refers to `VM_EP_LEP(reg_cfp->ep)` % # EXEC_EC_CFP(): refers to `val = vm_exec(ec, TRUE)` with frame setup % # CALL_METHOD(): using `GET_CFP()` and `EXEC_EC_CFP()` -% # TOPN(): refers to `reg_cfp->sp`, or `*(stack + (stack_size - num - 1))` if local_stack_p -% # STACK_ADDR_FROM_TOP(): refers to `reg_cfp->sp`, or `stack + (stack_size - num)` if local_stack_p +% # TOPN(): refers to `reg_cfp->sp` +% # STACK_ADDR_FROM_TOP(): refers to `reg_cfp->sp` % # DISPATCH_ORIGINAL_INSN(): expanded in _mjit_compile_insn.erb % # THROW_EXCEPTION(): specially defined for JIT % # RESTORE_REGS(): specially defined for `leave` Index: tool/ruby_vm/views/_mjit_compile_insn.erb =================================================================== --- tool/ruby_vm/views/_mjit_compile_insn.erb (revision 63862) +++ tool/ruby_vm/views/_mjit_compile_insn.erb (revision 63863) @@ -20,11 +20,6 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_insn.erb#L20 MAYBE_UNUSED(<%= ope.fetch(:decl) %>) = (<%= ope.fetch(:type) %>)operands[<%= i %>]; % end % -% # JIT: Declare stack_size to be used in some macro of _mjit_compile_insn_body.erb - if (status->local_stack_p) { - fprintf(f, " MAYBE_UNUSED(unsigned int) stack_size = %u;\n", b->stack_size); - } -% % # JIT: Declare variables for operands, popped values and return values % insn.declarations.each do |decl| fprintf(f, " <%= decl %>;\n"); Index: tool/ruby_vm/views/_mjit_compile_insn_body.erb =================================================================== --- tool/ruby_vm/views/_mjit_compile_insn_body.erb (revision 63862) +++ tool/ruby_vm/views/_mjit_compile_insn_body.erb (revision 63863) @@ -69,9 +69,6 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_insn_body.erb#L69 % end % when /\A\s+DISPATCH_ORIGINAL_INSN\([^)]+\);\s+\z/ % # For `opt_xxx`'s fallbacks. - if (status->local_stack_p) { - fprintf(f, " reg_cfp->sp = (VALUE *)reg_cfp->bp + %d;\n", b->stack_size + 1); - } if (!body->catch_except_p) { fprintf(f, " reg_cfp->pc = original_body_iseq + %d;\n", pos); } Index: tool/ruby_vm/views/_mjit_compile_pc_and_sp.erb =================================================================== --- tool/ruby_vm/views/_mjit_compile_pc_and_sp.erb (revision 63862) +++ tool/ruby_vm/views/_mjit_compile_pc_and_sp.erb (revision 63863) @@ -12,15 +12,4 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_pc_and_sp.erb#L12 } % % # JIT: move sp to use or preserve stack variables - if (status->local_stack_p) { -% # JIT-only behavior (pushing JIT's local variables to VM's stack): - rb_snum_t i, push_size; - push_size = -<%= insn.call_attribute('sp_inc') %> + <%= insn.rets.size %> - <%= insn.pops.size %>; - fprintf(f, " reg_cfp->sp = (VALUE *)reg_cfp->bp + %ld + 1;\n", push_size); /* POPN(INSN_ATTR(popn)); */ - for (i = 0; i < push_size; i++) { /* TODO: use memcpy? */ - fprintf(f, " *(reg_cfp->sp + %ld) = stack[%ld];\n", i - push_size, (rb_snum_t)b->stack_size - push_size + i); - } - } - else { - fprintf(f, " reg_cfp->sp = (VALUE *)reg_cfp->bp + %d;\n", b->stack_size + 1 - <%= insn.pops.size %>); /* POPN(INSN_ATTR(popn)); */ - } + fprintf(f, " reg_cfp->sp = (VALUE *)reg_cfp->bp + %d;\n", b->stack_size + 1 - <%= insn.pops.size %>); /* POPN(INSN_ATTR(popn)); */ Index: test/ruby/test_jit.rb =================================================================== --- test/ruby/test_jit.rb (revision 63862) +++ test/ruby/test_jit.rb (revision 63863) @@ -675,6 +675,24 @@ class TestJIT < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_jit.rb#L675 end; end + def test_stack_pointer_with_assignment + assert_eval_with_jit("#{<<~"begin;"}\n#{<<~"end;"}", stdout: "nil\nnil\n", success_count: 1) + begin; + 2.times do + a, b = nil + p a + end + end; + end + + def test_stack_pointer_with_regexpmatch + skip + assert_eval_with_jit("#{<<~"begin;"}\n#{<<~"end;"}", stdout: "nil\nnil\n", success_count: 1) + begin; + 1000.times { break if /a/ =~ "ab" && !$~[0] } + end; + end + private # The shortest way to test one proc -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/