8142984: Zero: fast accessors should handle both getters and setters

shipilev · shipilev · commit defdd12e70bb · 2020-11-19T17:37:07.000Z
Reviewed-by: andrew, coleenp
diff --git a/src/hotspot/cpu/zero/zeroInterpreter_zero.cpp b/src/hotspot/cpu/zero/zeroInterpreter_zero.cpp
@@ -547,38 +547,23 @@ int ZeroInterpreter::native_entry(Method* method, intptr_t UNUSED, TRAPS) {
   return 0;
 }
 
-int ZeroInterpreter::accessor_entry(Method* method, intptr_t UNUSED, TRAPS) {
-  JavaThread *thread = THREAD->as_Java_thread();
-  ZeroStack *stack = thread->zero_stack();
-  intptr_t *locals = stack->sp();
-
+int ZeroInterpreter::getter_entry(Method* method, intptr_t UNUSED, TRAPS) {
   // Drop into the slow path if we need a safepoint check
   if (SafepointMechanism::should_process(THREAD)) {
     return normal_entry(method, 0, THREAD);
   }
 
-  // Load the object pointer and drop into the slow path
-  // if we have a NullPointerException
-  oop object = LOCALS_OBJECT(0);
-  if (object == NULL) {
-    return normal_entry(method, 0, THREAD);
-  }
-
-  // Read the field index from the bytecode, which looks like this:
+  // Read the field index from the bytecode:
   //  0:  aload_0
   //  1:  getfield
   //  2:    index
   //  3:    index
-  //  4:  ireturn/areturn/freturn/lreturn/dreturn
+  //  4:  return
+  //
   // NB this is not raw bytecode: index is in machine order
-  u1 *code = method->code_base();
-  assert(code[0] == Bytecodes::_aload_0 &&
-         code[1] == Bytecodes::_getfield &&
-         (code[4] == Bytecodes::_ireturn ||
-          code[4] == Bytecodes::_freturn ||
-          code[4] == Bytecodes::_lreturn ||
-          code[4] == Bytecodes::_dreturn ||
-          code[4] == Bytecodes::_areturn), "should do");
+
+  assert(method->is_getter(), "Expect the particular bytecode shape");
+  u1* code = method->code_base();
   u2 index = Bytes::get_native_u2(&code[2]);
 
   // Get the entry from the constant pool cache, and drop into
@@ -589,96 +574,154 @@ int ZeroInterpreter::accessor_entry(Method* method, intptr_t UNUSED, TRAPS) {
     return normal_entry(method, 0, THREAD);
   }
 
-  // Get the result and push it onto the stack
+  JavaThread* thread = THREAD->as_Java_thread();
+  ZeroStack* stack = thread->zero_stack();
+  intptr_t* topOfStack = stack->sp();
+
+  // Load the object pointer and drop into the slow path
+  // if we have a NullPointerException
+  oop object = STACK_OBJECT(0);
+  if (object == NULL) {
+    return normal_entry(method, 0, THREAD);
+  }
+
+  // If needed, allocate additional slot on stack: we already have one
+  // for receiver, and double/long need another one.
   switch (entry->flag_state()) {
-  case ltos:
-  case dtos:
-    stack->overflow_check(1, CHECK_0);
-    stack->alloc(wordSize);
-    break;
+    case ltos:
+    case dtos:
+      stack->overflow_check(1, CHECK_0);
+      stack->alloc(wordSize);
+      topOfStack = stack->sp();
+      break;
   }
+
+  // Read the field to stack(0)
+  int offset = entry->f2_as_index();
   if (entry->is_volatile()) {
     if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
       OrderAccess::fence();
     }
     switch (entry->flag_state()) {
-    case ctos:
-      SET_LOCALS_INT(object->char_field_acquire(entry->f2_as_index()), 0);
-      break;
-
-    case btos:
-    case ztos:
-      SET_LOCALS_INT(object->byte_field_acquire(entry->f2_as_index()), 0);
-      break;
-
-    case stos:
-      SET_LOCALS_INT(object->short_field_acquire(entry->f2_as_index()), 0);
-      break;
-
-    case itos:
-      SET_LOCALS_INT(object->int_field_acquire(entry->f2_as_index()), 0);
-      break;
+      case btos:
+      case ztos: SET_STACK_INT(object->byte_field_acquire(offset),      0); break;
+      case ctos: SET_STACK_INT(object->char_field_acquire(offset),      0); break;
+      case stos: SET_STACK_INT(object->short_field_acquire(offset),     0); break;
+      case itos: SET_STACK_INT(object->int_field_acquire(offset),       0); break;
+      case ltos: SET_STACK_LONG(object->long_field_acquire(offset),     0); break;
+      case ftos: SET_STACK_FLOAT(object->float_field_acquire(offset),   0); break;
+      case dtos: SET_STACK_DOUBLE(object->double_field_acquire(offset), 0); break;
+      case atos: SET_STACK_OBJECT(object->obj_field_acquire(offset),    0); break;
+      default:
+        ShouldNotReachHere();
+    }
+  } else {
+    switch (entry->flag_state()) {
+      case btos:
+      case ztos: SET_STACK_INT(object->byte_field(offset),      0); break;
+      case ctos: SET_STACK_INT(object->char_field(offset),      0); break;
+      case stos: SET_STACK_INT(object->short_field(offset),     0); break;
+      case itos: SET_STACK_INT(object->int_field(offset),       0); break;
+      case ltos: SET_STACK_LONG(object->long_field(offset),     0); break;
+      case ftos: SET_STACK_FLOAT(object->float_field(offset),   0); break;
+      case dtos: SET_STACK_DOUBLE(object->double_field(offset), 0); break;
+      case atos: SET_STACK_OBJECT(object->obj_field(offset),    0); break;
+      default:
+        ShouldNotReachHere();
+    }
+  }
 
-    case ltos:
-      SET_LOCALS_LONG(object->long_field_acquire(entry->f2_as_index()), 0);
-      break;
+  // No deoptimized frames on the stack
+  return 0;
+}
 
-    case ftos:
-      SET_LOCALS_FLOAT(object->float_field_acquire(entry->f2_as_index()), 0);
-      break;
+int ZeroInterpreter::setter_entry(Method* method, intptr_t UNUSED, TRAPS) {
+  // Drop into the slow path if we need a safepoint check
+  if (SafepointMechanism::should_process(THREAD)) {
+    return normal_entry(method, 0, THREAD);
+  }
 
-    case dtos:
-      SET_LOCALS_DOUBLE(object->double_field_acquire(entry->f2_as_index()), 0);
-      break;
+  // Read the field index from the bytecode:
+  //  0:  aload_0
+  //  1:  *load_1
+  //  2:  putfield
+  //  3:    index
+  //  4:    index
+  //  5:  return
+  //
+  // NB this is not raw bytecode: index is in machine order
 
-    case atos:
-      SET_LOCALS_OBJECT(object->obj_field_acquire(entry->f2_as_index()), 0);
-      break;
+  assert(method->is_setter(), "Expect the particular bytecode shape");
+  u1* code = method->code_base();
+  u2 index = Bytes::get_native_u2(&code[3]);
 
-    default:
-      ShouldNotReachHere();
-    }
+  // Get the entry from the constant pool cache, and drop into
+  // the slow path if it has not been resolved
+  ConstantPoolCache* cache = method->constants()->cache();
+  ConstantPoolCacheEntry* entry = cache->entry_at(index);
+  if (!entry->is_resolved(Bytecodes::_putfield)) {
+    return normal_entry(method, 0, THREAD);
   }
-  else {
-    switch (entry->flag_state()) {
-    case ctos:
-      SET_LOCALS_INT(object->char_field(entry->f2_as_index()), 0);
-      break;
 
-    case btos:
-    case ztos:
-      SET_LOCALS_INT(object->byte_field(entry->f2_as_index()), 0);
-      break;
-
-    case stos:
-      SET_LOCALS_INT(object->short_field(entry->f2_as_index()), 0);
-      break;
-
-    case itos:
-      SET_LOCALS_INT(object->int_field(entry->f2_as_index()), 0);
-      break;
+  JavaThread* thread = THREAD->as_Java_thread();
+  ZeroStack* stack = thread->zero_stack();
+  intptr_t* topOfStack = stack->sp();
 
+  // Figure out where the receiver is. If there is a long/double
+  // operand on stack top, then receiver is two slots down.
+  oop object = NULL;
+  switch (entry->flag_state()) {
     case ltos:
-      SET_LOCALS_LONG(object->long_field(entry->f2_as_index()), 0);
-      break;
-
-    case ftos:
-      SET_LOCALS_FLOAT(object->float_field(entry->f2_as_index()), 0);
-      break;
-
     case dtos:
-      SET_LOCALS_DOUBLE(object->double_field(entry->f2_as_index()), 0);
+      object = STACK_OBJECT(-2);
       break;
-
-    case atos:
-      SET_LOCALS_OBJECT(object->obj_field(entry->f2_as_index()), 0);
+    default:
+      object = STACK_OBJECT(-1);
       break;
+  }
 
-    default:
-      ShouldNotReachHere();
+  // Load the receiver pointer and drop into the slow path
+  // if we have a NullPointerException
+  if (object == NULL) {
+    return normal_entry(method, 0, THREAD);
+  }
+
+  // Store the stack(0) to field
+  int offset = entry->f2_as_index();
+  if (entry->is_volatile()) {
+    switch (entry->flag_state()) {
+      case btos: object->release_byte_field_put(offset,   STACK_INT(0));     break;
+      case ztos: object->release_byte_field_put(offset,   STACK_INT(0) & 1); break; // only store LSB
+      case ctos: object->release_char_field_put(offset,   STACK_INT(0));     break;
+      case stos: object->release_short_field_put(offset,  STACK_INT(0));     break;
+      case itos: object->release_int_field_put(offset,    STACK_INT(0));     break;
+      case ltos: object->release_long_field_put(offset,   STACK_LONG(0));    break;
+      case ftos: object->release_float_field_put(offset,  STACK_FLOAT(0));   break;
+      case dtos: object->release_double_field_put(offset, STACK_DOUBLE(0));  break;
+      case atos: object->release_obj_field_put(offset,    STACK_OBJECT(0));  break;
+      default:
+        ShouldNotReachHere();
+    }
+    OrderAccess::storeload();
+  } else {
+    switch (entry->flag_state()) {
+      case btos: object->byte_field_put(offset,   STACK_INT(0));     break;
+      case ztos: object->byte_field_put(offset,   STACK_INT(0) & 1); break; // only store LSB
+      case ctos: object->char_field_put(offset,   STACK_INT(0));     break;
+      case stos: object->short_field_put(offset,  STACK_INT(0));     break;
+      case itos: object->int_field_put(offset,    STACK_INT(0));     break;
+      case ltos: object->long_field_put(offset,   STACK_LONG(0));    break;
+      case ftos: object->float_field_put(offset,  STACK_FLOAT(0));   break;
+      case dtos: object->double_field_put(offset, STACK_DOUBLE(0));  break;
+      case atos: object->obj_field_put(offset,    STACK_OBJECT(0));  break;
+      default:
+        ShouldNotReachHere();
     }
   }
 
+  // Nothing is returned, pop out parameters
+  stack->set_sp(stack->sp() + method->size_of_parameters());
+
   // No deoptimized frames on the stack
   return 0;
 }
diff --git a/src/hotspot/cpu/zero/zeroInterpreter_zero.hpp b/src/hotspot/cpu/zero/zeroInterpreter_zero.hpp
@@ -34,7 +34,8 @@
   // Method entries
   static int normal_entry(Method* method, intptr_t UNUSED, TRAPS);
   static int native_entry(Method* method, intptr_t UNUSED, TRAPS);
-  static int accessor_entry(Method* method, intptr_t UNUSED, TRAPS);
+  static int getter_entry(Method* method, intptr_t UNUSED, TRAPS);
+  static int setter_entry(Method* method, intptr_t UNUSED, TRAPS);
   static int empty_entry(Method* method, intptr_t UNUSED, TRAPS);
 
  public:
diff --git a/src/hotspot/share/interpreter/abstractInterpreter.cpp b/src/hotspot/share/interpreter/abstractInterpreter.cpp
@@ -185,13 +185,14 @@ AbstractInterpreter::MethodKind AbstractInterpreter::method_kind(const methodHan
     default                   : break;
   }
 
-  // Accessor method?
+  // Getter method?
   if (m->is_getter()) {
-    // TODO: We should have used ::is_accessor above, but fast accessors in Zero expect only getters.
-    // See ZeroInterpreter::accessor_entry in zeroInterpreter_zero.cpp. This should be fixed in Zero,
-    // then the call above updated to ::is_accessor
-    assert(m->size_of_parameters() == 1, "fast code for accessors assumes parameter size = 1");
-    return accessor;
+    return getter;
+  }
+
+  // Setter method?
+  if (m->is_setter()) {
+    return setter;
   }
 
   // Note: for now: zero locals for all non-empty methods
@@ -300,7 +301,8 @@ void AbstractInterpreter::print_method_kind(MethodKind kind) {
     case native                 : tty->print("native"                 ); break;
     case native_synchronized    : tty->print("native_synchronized"    ); break;
     case empty                  : tty->print("empty"                  ); break;
-    case accessor               : tty->print("accessor"               ); break;
+    case getter                 : tty->print("getter"                 ); break;
+    case setter                 : tty->print("setter"                 ); break;
     case abstract               : tty->print("abstract"               ); break;
     case java_lang_math_sin     : tty->print("java_lang_math_sin"     ); break;
     case java_lang_math_cos     : tty->print("java_lang_math_cos"     ); break;
diff --git a/src/hotspot/share/interpreter/abstractInterpreter.hpp b/src/hotspot/share/interpreter/abstractInterpreter.hpp
@@ -60,7 +60,8 @@ class AbstractInterpreter: AllStatic {
     native,                                                     // native method
     native_synchronized,                                        // native method & is synchronized
     empty,                                                      // empty method (code: _return)
-    accessor,                                                   // accessor method (code: _aload_0, _getfield, _(a|i)return)
+    getter,                                                     // getter method
+    setter,                                                     // setter method
     abstract,                                                   // abstract method (throws an AbstractMethodException)
     method_handle_invoke_FIRST,                                 // java.lang.invoke.MethodHandles::invokeExact, etc.
     method_handle_invoke_LAST                                   = (method_handle_invoke_FIRST
diff --git a/src/hotspot/share/interpreter/templateInterpreterGenerator.cpp b/src/hotspot/share/interpreter/templateInterpreterGenerator.cpp
@@ -188,7 +188,8 @@ void TemplateInterpreterGenerator::generate_all() {
   method_entry(zerolocals)
   method_entry(zerolocals_synchronized)
   method_entry(empty)
-  method_entry(accessor)
+  method_entry(getter)
+  method_entry(setter)
   method_entry(abstract)
   method_entry(java_lang_math_sin  )
   method_entry(java_lang_math_cos  )
@@ -406,7 +407,8 @@ address TemplateInterpreterGenerator::generate_method_entry(
   case Interpreter::native                 : native = true;                           break;
   case Interpreter::native_synchronized    : native = true; synchronized = true;      break;
   case Interpreter::empty                  : break;
-  case Interpreter::accessor               : break;
+  case Interpreter::getter                 : break;
+  case Interpreter::setter                 : break;
   case Interpreter::abstract               : entry_point = generate_abstract_entry(); break;
 
   case Interpreter::java_lang_math_sin     : // fall thru
diff --git a/src/hotspot/share/interpreter/zero/zeroInterpreterGenerator.cpp b/src/hotspot/share/interpreter/zero/zeroInterpreterGenerator.cpp
@@ -50,7 +50,8 @@ void ZeroInterpreterGenerator::generate_all() {
     method_entry(zerolocals);
     method_entry(zerolocals_synchronized);
     method_entry(empty);
-    method_entry(accessor);
+    method_entry(getter);
+    method_entry(setter);
     method_entry(abstract);
     method_entry(java_lang_math_sin   );
     method_entry(java_lang_math_cos   );
@@ -90,7 +91,8 @@ address ZeroInterpreterGenerator::generate_method_entry(
   case Interpreter::native                 : native = true;                           break;
   case Interpreter::native_synchronized    : native = true; synchronized = true;      break;
   case Interpreter::empty                  : entry_point = generate_empty_entry();    break;
-  case Interpreter::accessor               : entry_point = generate_accessor_entry(); break;
+  case Interpreter::getter                 : entry_point = generate_getter_entry();   break;
+  case Interpreter::setter                 : entry_point = generate_setter_entry();   break;
   case Interpreter::abstract               : entry_point = generate_abstract_entry(); break;
 
   case Interpreter::java_lang_math_sin     : // fall thru
@@ -156,11 +158,18 @@ address ZeroInterpreterGenerator::generate_empty_entry() {
   return generate_entry((address) ZeroInterpreter::empty_entry);
 }
 
-address ZeroInterpreterGenerator::generate_accessor_entry() {
+address ZeroInterpreterGenerator::generate_getter_entry() {
   if (!UseFastAccessorMethods)
     return NULL;
 
-  return generate_entry((address) ZeroInterpreter::accessor_entry);
+  return generate_entry((address) ZeroInterpreter::getter_entry);
+}
+
+address ZeroInterpreterGenerator::generate_setter_entry() {
+  if (!UseFastAccessorMethods)
+    return NULL;
+
+  return generate_entry((address) ZeroInterpreter::setter_entry);
 }
 
 address ZeroInterpreterGenerator::generate_Reference_get_entry(void) {
diff --git a/src/hotspot/share/interpreter/zero/zeroInterpreterGenerator.hpp b/src/hotspot/share/interpreter/zero/zeroInterpreterGenerator.hpp
@@ -44,7 +44,8 @@ class ZeroInterpreterGenerator: public AbstractInterpreterGenerator {
   address generate_abstract_entry();
   address generate_math_entry(AbstractInterpreter::MethodKind kind);
   address generate_empty_entry();
-  address generate_accessor_entry();
+  address generate_getter_entry();
+  address generate_setter_entry();
   address generate_Reference_get_entry();
 
  public:
