Merge pull request #172 from jhawthorn/invokesuper_me

Implement invokesuper using cfp->ep[ME] check

bootstraptest/test_yjit.rb

@@ -1444,6 +1444,85 @@ def foo = [:B, super]
   ins.bar
 }
 
+# invokesuper changed ancestor
+assert_equal '[:A, [:M, :B]]', %q{
+  class B
+    def foo
+      :B
+    end
+  end
+
+  class A < B
+    def foo
+      [:A, super]
+    end
+  end
+
+  module M
+    def foo
+      [:M, super]
+    end
+  end
+
+  ins = A.new
+  ins.foo
+  ins.foo
+  A.include(M)
+  ins.foo
+}
+
+# invokesuper changed ancestor via prepend
+assert_equal '[:A, [:M, :B]]', %q{
+  class B
+    def foo
+      :B
+    end
+  end
+
+  class A < B
+    def foo
+      [:A, super]
+    end
+  end
+
+  module M
+    def foo
+      [:M, super]
+    end
+  end
+
+  ins = A.new
+  ins.foo
+  ins.foo
+  B.prepend(M)
+  ins.foo
+}
+
+# invokesuper replaced method
+assert_equal '[:A, :Btwo]', %q{
+  class B
+    def foo
+      :B
+    end
+  end
+
+  class A < B
+    def foo
+      [:A, super]
+    end
+  end
+
+  ins = A.new
+  ins.foo
+  ins.foo
+  class B
+    def foo
+      :Btwo
+    end
+  end
+  ins.foo
+}
+
 # Call to fixnum
 assert_equal '[true, false]', %q{
   def is_odd(obj)

test/ruby/test_yjit.rb

@@ -162,6 +162,40 @@ def make_str(foo, bar)
     RUBY
   end
 
+  def test_super_iseq
+    assert_compiles(<<~'RUBY', insns: %i[invokesuper opt_plus opt_mult], result: 15)
+      class A
+        def foo
+          1 + 2
+        end
+      end
+
+      class B < A
+        def foo
+          super * 5
+        end
+      end
+
+      B.new.foo
+    RUBY
+  end
+
+  def test_super_cfunc
+    assert_compiles(<<~'RUBY', insns: %i[invokesuper], result: "Hello")
+      class Gnirts < String
+        def initialize
+          super(-"olleH")
+        end
+
+        def to_s
+          super().reverse
+        end
+      end
+
+      Gnirts.new.to_s
+    RUBY
+  end
+
   def test_fib_recursion
     assert_compiles(<<~'RUBY', insns: %i[opt_le opt_minus opt_plus opt_send_without_block], result: 34)
       def fib(n)

yjit.rb

@@ -159,6 +159,7 @@ def _print_stats
       $stderr.puts("Number of locals modified through binding: %d\n" % stats[:binding_set])
 
       print_counters(stats, prefix: 'send_', prompt: 'method call exit reasons: ')
+      print_counters(stats, prefix: 'invokesuper_', prompt: 'invokesuper exit reasons: ')
       print_counters(stats, prefix: 'leave_', prompt: 'leave exit reasons: ')
       print_counters(stats, prefix: 'getivar_', prompt: 'getinstancevariable exit reasons:')
       print_counters(stats, prefix: 'setivar_', prompt: 'setinstancevariable exit reasons:')

yjit_codegen.c

@@ -3457,8 +3457,6 @@ gen_send(jitstate_t *jit, ctx_t *ctx)
     return gen_send_general(jit, ctx, cd, block);
 }
 
-// Not in use as it's incorrect in some situations. See comments.
-RBIMPL_ATTR_MAYBE_UNUSED()
 static codegen_status_t
 gen_invokesuper(jitstate_t *jit, ctx_t *ctx)
 {
@@ -3474,17 +3472,19 @@ gen_invokesuper(jitstate_t *jit, ctx_t *ctx)
     const rb_callable_method_entry_t *me = rb_vm_frame_method_entry(jit->ec->cfp);
     if (!me) {
         return YJIT_CANT_COMPILE;
-    } else if (me->def->type == VM_METHOD_TYPE_BMETHOD) {
-        // In the interpreter the method id can change which is tested for and
-        // invalidates the cache.
-        // By skipping super calls inside a BMETHOD definition, I believe we
-        // avoid this case
-        return YJIT_CANT_COMPILE;
     }
 
+    // FIXME: We should track and invalidate this block when this cme is invalidated
     VALUE current_defined_class = me->defined_class;
     ID mid = me->def->original_id;
 
+    if (me != rb_callable_method_entry(current_defined_class, me->called_id)) {
+        // Though we likely could generate this call, as we are only concerned
+        // with the method entry remaining valid, assume_method_lookup_stable
+        // below requires that the method lookup matches as well
+        return YJIT_CANT_COMPILE;
+    }
+
     // vm_search_normal_superclass
     if (BUILTIN_TYPE(current_defined_class) == T_ICLASS && FL_TEST_RAW(RBASIC(current_defined_class)->klass, RMODULE_IS_REFINEMENT)) {
         return YJIT_CANT_COMPILE;
@@ -3501,25 +3501,16 @@ gen_invokesuper(jitstate_t *jit, ctx_t *ctx)
         return YJIT_CANT_COMPILE;
     }
 
-    VALUE comptime_recv = jit_peek_at_stack(jit, ctx, argc);
-    VALUE comptime_recv_klass = CLASS_OF(comptime_recv);
-
     // Ensure we haven't rebound this method onto an incompatible class.
     // In the interpreter we try to avoid making this check by performing some
-    // cheaper calculations first, but since we specialize on the receiver
-    // class and so only have to do this once at compile time this is fine to
-    // always check and side exit.
+    // cheaper calculations first, but since we specialize on the method entry
+    // and so only have to do this once at compile time this is fine to always
+    // check and side exit.
+    VALUE comptime_recv = jit_peek_at_stack(jit, ctx, argc);
     if (!rb_obj_is_kind_of(comptime_recv, current_defined_class)) {
         return YJIT_CANT_COMPILE;
     }
 
-    // Because we're assuming only one current_defined_class for a given
-    // receiver class we need to check that the superclass doesn't also
-    // re-include the same module.
-    if (rb_class_search_ancestor(comptime_superclass, current_defined_class)) {
-        return YJIT_CANT_COMPILE;
-    }
-
     // Do method lookup
     const rb_callable_method_entry_t *cme = rb_callable_method_entry(comptime_superclass, mid);
 
@@ -3540,6 +3531,18 @@ gen_invokesuper(jitstate_t *jit, ctx_t *ctx)
     // Guard that the receiver has the same class as the one from compile time
     uint8_t *side_exit = yjit_side_exit(jit, ctx);
 
+    if (jit->ec->cfp->ep[VM_ENV_DATA_INDEX_ME_CREF] != (VALUE)me) {
+        // This will be the case for super within a block
+        return YJIT_CANT_COMPILE;
+    }
+
+    ADD_COMMENT(cb, "guard known me");
+    mov(cb, REG0, member_opnd(REG_CFP, rb_control_frame_t, ep));
+    x86opnd_t ep_me_opnd = mem_opnd(64, REG0, SIZEOF_VALUE * VM_ENV_DATA_INDEX_ME_CREF);
+    jit_mov_gc_ptr(jit, cb, REG1, (VALUE)me);
+    cmp(cb, ep_me_opnd, REG1);
+    jne_ptr(cb, COUNTED_EXIT(side_exit, invokesuper_me_changed));
+
     if (!block) {
         // Guard no block passed
         // rb_vm_frame_block_handler(GET_EC()->cfp) == VM_BLOCK_HANDLER_NONE
@@ -3548,25 +3551,20 @@ gen_invokesuper(jitstate_t *jit, ctx_t *ctx)
         // TODO: this could properly forward the current block handler, but
         // would require changes to gen_send_*
         ADD_COMMENT(cb, "guard no block given");
-        mov(cb, REG0, member_opnd(REG_CFP, rb_control_frame_t, ep));
-        mov(cb, REG0, mem_opnd(64, REG0, SIZEOF_VALUE * VM_ENV_DATA_INDEX_SPECVAL));
-        cmp(cb, REG0, imm_opnd(VM_BLOCK_HANDLER_NONE));
-        jne_ptr(cb, side_exit);
+        // EP is in REG0 from above
+        x86opnd_t ep_specval_opnd = mem_opnd(64, REG0, SIZEOF_VALUE * VM_ENV_DATA_INDEX_SPECVAL);
+        cmp(cb, ep_specval_opnd, imm_opnd(VM_BLOCK_HANDLER_NONE));
+        jne_ptr(cb, COUNTED_EXIT(side_exit, invokesuper_block));
     }
 
     // Points to the receiver operand on the stack
     x86opnd_t recv = ctx_stack_opnd(ctx, argc);
-    insn_opnd_t recv_opnd = OPND_STACK(argc);
     mov(cb, REG0, recv);
 
-    // FIXME: This guard and the assume_method_lookup_stable() call below isn't
-    // always enough to correctly replicate the interpreter's behavior of
-    // searching at runtime for the callee through the method entry of the stack frame.
-    if (!jit_guard_known_klass(jit, ctx, comptime_recv_klass, recv_opnd, comptime_recv, SEND_MAX_DEPTH, side_exit)) {
-        return YJIT_CANT_COMPILE;
-    }
-
-    assume_method_lookup_stable(comptime_recv_klass, cme, jit->block);
+    // We need to assume that both our current method entry and the super
+    // method entry we invoke remain stable
+    assume_method_lookup_stable(current_defined_class, me, jit->block);
+    assume_method_lookup_stable(comptime_superclass, cme, jit->block);
 
     // Method calls may corrupt types
     ctx_clear_local_types(ctx);
@@ -4065,6 +4063,7 @@ yjit_init_codegen(void)
     yjit_reg_op(BIN(getblockparamproxy), gen_getblockparamproxy);
     yjit_reg_op(BIN(opt_send_without_block), gen_opt_send_without_block);
     yjit_reg_op(BIN(send), gen_send);
+    yjit_reg_op(BIN(invokesuper), gen_invokesuper);
     yjit_reg_op(BIN(leave), gen_leave);
     yjit_reg_op(BIN(getglobal), gen_getglobal);
     yjit_reg_op(BIN(setglobal), gen_setglobal);

yjit_core.c

@@ -541,8 +541,11 @@ add_block_version(blockid_t blockid, block_t* block)
     {
         // By writing the new block to the iseq, the iseq now
         // contains new references to Ruby objects. Run write barriers.
-        RB_OBJ_WRITTEN(iseq, Qundef, block->receiver_klass);
-        RB_OBJ_WRITTEN(iseq, Qundef, block->callee_cme);
+        cme_dependency_t *cme_dep;
+        rb_darray_foreach(block->cme_dependencies, cme_dependency_idx, cme_dep) {
+            RB_OBJ_WRITTEN(iseq, Qundef, cme_dep->receiver_klass);
+            RB_OBJ_WRITTEN(iseq, Qundef, cme_dep->callee_cme);
+        }
 
         // Run write barriers for all objects in generated code.
         uint32_t *offset_element;

yjit_core.h

@@ -212,6 +212,15 @@ typedef struct yjit_branch_entry
 
 } branch_t;
 
+// In case this block is invalidated, these two pieces of info
+// help to remove all pointers to this block in the system.
+typedef struct {
+    VALUE receiver_klass;
+    VALUE callee_cme;
+} cme_dependency_t;
+
+typedef rb_darray(cme_dependency_t) cme_dependency_array_t;
+
 typedef rb_darray(branch_t*) branch_array_t;
 
 typedef rb_darray(uint32_t) int32_array_t;
@@ -243,10 +252,9 @@ typedef struct yjit_block_version
     // Offsets for GC managed objects in the mainline code block
     int32_array_t gc_object_offsets;
 
-    // In case this block is invalidated, these two pieces of info
-    // help to remove all pointers to this block in the system.
-    VALUE receiver_klass;
-    VALUE callee_cme;
+    // CME dependencies of this block, to help to remove all pointers to this
+    // block in the system.
+    cme_dependency_array_t cme_dependencies;
 
     // Code page this block lives on
     VALUE code_page;