changeset 54493:cf9a8e7a0094 lworld

8220118: [lworld] Fix C1 handling of unloaded Q classes Reviewed-by: thartmann
author iklam
date Wed, 06 Mar 2019 21:30:53 -0800
parents a1bdb6bccfdd
children 42adc57b994f
files src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp src/hotspot/cpu/x86/c1_Runtime1_x86.cpp src/hotspot/share/c1/c1_CodeStubs.hpp src/hotspot/share/c1/c1_GraphBuilder.cpp src/hotspot/share/c1/c1_Instruction.cpp src/hotspot/share/c1/c1_LIRGenerator.cpp src/hotspot/share/ci/ciEnv.cpp src/hotspot/share/ci/ciEnv.hpp src/hotspot/share/ci/ciObjectFactory.cpp src/hotspot/share/ci/ciObjectFactory.hpp src/hotspot/share/ci/ciValueArrayKlass.cpp test/hotspot/jtreg/compiler/valhalla/valuetypes/TestUnloadedValueTypeArray.java
diffstat 13 files changed, 142 insertions(+), 79 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -187,6 +187,8 @@
   _array = array;
   _index = index;
   _value = value;
+  // Tell the register allocator that the runtime call will scratch rax.
+  _scratch_reg = FrameMap::rax_oop_opr;
   _info = new CodeEmitInfo(info);
 }
 
--- a/src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -1305,12 +1305,12 @@
   LIR_Opr len = length.result();
 
   ciKlass* obj = (ciKlass*) x->exact_type();
-  CodeStub* slow_path = new NewObjectArrayStub(klass_reg, len, reg, info, obj->is_value_array_klass());
+  CodeStub* slow_path = new NewObjectArrayStub(klass_reg, len, reg, info, x->is_never_null());
   if (obj == ciEnv::unloaded_ciobjarrayklass()) {
     BAILOUT("encountered unloaded_ciobjarrayklass due to out of memory error");
   }
   klass2reg_with_patching(klass_reg, obj, patching_info);
-  if (obj->is_value_array_klass()) {
+  if (x->is_never_null()) {
     __ allocate_array(reg, len, tmp1, tmp2, tmp3, tmp4, T_VALUETYPE, klass_reg, slow_path);
   } else {
     __ allocate_array(reg, len, tmp1, tmp2, tmp3, tmp4, T_OBJECT, klass_reg, slow_path);
--- a/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1125,13 +1125,27 @@
           __ movl(t0, Address(klass, Klass::layout_helper_offset()));
           __ sarl(t0, Klass::_lh_array_tag_shift);
           switch (id) {
-          case new_type_array_id:    __ cmpl(t0, Klass::_lh_array_tag_type_value); break;
-          case new_object_array_id:  __ cmpl(t0, Klass::_lh_array_tag_obj_value);  break;
-          case new_value_array_id:   __ cmpl(t0, Klass::_lh_array_tag_vt_value);   break;
+          case new_type_array_id:
+            __ cmpl(t0, Klass::_lh_array_tag_type_value);
+            __ jcc(Assembler::equal, ok);
+            __ stop("assert(is a type array klass)");
+            break;
+          case new_object_array_id:
+          case new_value_array_id: // <-- needs to be renamed to new_non_null_array_id!
+            // FIXME:
+            // The VM currently does not distinguish between anewarray of
+            // "[QV;" (elements are non-nullable) vs "[LV;" (elements may be null).
+            // Instead, both are treated essentially as "[QV;". This code needs
+            // to be reimplemented after proper support of "[LV;" is implemented in the VM.
+            //
+            __ cmpl(t0, Klass::_lh_array_tag_obj_value);
+            __ jcc(Assembler::equal, ok);
+            __ cmpl(t0, Klass::_lh_array_tag_vt_value);
+            __ jcc(Assembler::equal, ok);
+            __ stop("assert(is an object or value array klass)");
+            break;
           default:  ShouldNotReachHere();
           }
-          __ jcc(Assembler::equal, ok);
-          __ stop("assert(is an array klass)");
           __ should_not_reach_here();
           __ bind(ok);
         }
@@ -1185,7 +1199,7 @@
           call_offset = __ call_RT(obj, noreg, CAST_FROM_FN_PTR(address, new_type_array), klass, length);
         } else {
           // Runtime1::new_object_array handles both object and value arrays.
-          // new_value_array_id is needed only for the ASSERT block above.
+          // See comments in the ASSERT block above.
           call_offset = __ call_RT(obj, noreg, CAST_FROM_FN_PTR(address, new_object_array), klass, length);
         }
 
--- a/src/hotspot/share/c1/c1_CodeStubs.hpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/c1/c1_CodeStubs.hpp	Wed Mar 06 21:30:53 2019 -0800
@@ -252,10 +252,7 @@
     if (_scratch_reg != LIR_OprFact::illegalOpr) {
       visitor->do_temp(_scratch_reg);
     }
-  // Tell the register allocator that the runtime call will scratch rax.
-  visitor->do_output(FrameMap::rax_oop_opr);
-}
-
+  }
 
 #ifndef PRODUCT
   virtual void print_name(outputStream* out) const { out->print("LoadFlattenedArrayStub"); }
@@ -268,6 +265,7 @@
   LIR_Opr          _array;
   LIR_Opr          _index;
   LIR_Opr          _value;
+  LIR_Opr          _scratch_reg;
   CodeEmitInfo*    _info;
 
  public:
@@ -279,6 +277,9 @@
     visitor->do_input(_array);
     visitor->do_input(_index);
     visitor->do_input(_value);
+    if (_scratch_reg != LIR_OprFact::illegalOpr) {
+      visitor->do_temp(_scratch_reg);
+    }
   }
 #ifndef PRODUCT
   virtual void print_name(outputStream* out) const { out->print("StoreFlattenedArrayStub"); }
--- a/src/hotspot/share/c1/c1_GraphBuilder.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -2321,6 +2321,9 @@
   ciKlass* klass = stream()->get_klass(will_link);
   ValueStack* state_before = !klass->is_loaded() || PatchALot ? copy_state_before() : copy_state_exhandling();
   NewArray* n = new NewObjectArray(klass, ipop(), state_before);
+  if (stream()->is_klass_never_null()) {
+    n->set_never_null(true);
+  }
   apush(append_split(n));
 }
 
--- a/src/hotspot/share/c1/c1_Instruction.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/c1/c1_Instruction.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -115,16 +115,15 @@
 }
 
 
-// FIXME -- make this obsolete. Use maybe_flattened_array() or check_flattened_array() instead.
+// FIXME -- this is used by ValueStack::merge_types only. We should remove this function
+// and use a better way for handling phi nodes.
 bool Instruction::is_flattened_array() const {
   if (ValueArrayFlatten) {
     ciType* type = declared_type();
     if (type != NULL && type->is_value_array_klass()) {
       ciValueKlass* element_klass = type->as_value_array_klass()->element_klass()->as_value_klass();
-      if (!element_klass->is_loaded() || element_klass->flatten_array()) {
-        // Assume that all unloaded value arrays are not flattenable. If they
-        // turn out to be flattenable, we deoptimize on aaload/aastore.
-        // ^^^^ uugh -- this is ugly!
+      assert(element_klass->is_loaded(), "ciValueKlasses are always loaded");
+      if (element_klass->flatten_array()) {
         return true;
       }
     }
@@ -138,7 +137,8 @@
     ciType* type = declared_type();
     if (type != NULL && type->is_value_array_klass()) {
       ciValueKlass* element_klass = type->as_value_array_klass()->element_klass()->as_value_klass();
-      if (element_klass->is_loaded() && element_klass->flatten_array()) {
+      assert(element_klass->is_loaded(), "ciValueKlasses are always loaded");
+      if (element_klass->flatten_array()) {
         return true;
       }
     }
@@ -153,13 +153,13 @@
     if (type != NULL) {
       if (type->is_value_array_klass()) {
         ciValueKlass* element_klass = type->as_value_array_klass()->element_klass()->as_value_klass();
-        if (!element_klass->is_loaded() || element_klass->flatten_array()) {
-          // For unloaded value arrays, we will add a runtime check for flat-ness.
+        assert(element_klass->is_loaded(), "ciValueKlasses are always loaded");
+        if (element_klass->flatten_array()) {
           return true;
         }
       } else if (type->is_obj_array_klass()) {
         ciKlass* element_klass = type->as_obj_array_klass()->element_klass();
-        if (element_klass->is_java_lang_Object() || element_klass->is_interface()) {
+        if (!element_klass->is_loaded() || element_klass->is_java_lang_Object() || element_klass->is_interface()) {
           // Array covariance:
           //    (ValueType[] <: Object[])
           //    (ValueType[] <: <any interface>[])
--- a/src/hotspot/share/c1/c1_LIRGenerator.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/c1/c1_LIRGenerator.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -1543,8 +1543,9 @@
   if (x->needs_null_check() &&
       (needs_patching ||
        MacroAssembler::needs_explicit_null_check(x->offset()))) {
-    if (needs_patching && field_type == T_VALUETYPE) {
-      // We are storing a "Q" field, but the holder class is not yet loaded.
+    if (needs_patching && x->field()->signature()->starts_with("Q", 1)) {
+      // We are storing a field of type "QT;", but T is not yet loaded, so we don't
+      // know whether this field is flattened or not. Let's deoptimize and recompile.
       CodeStub* stub = new DeoptimizeStub(new CodeEmitInfo(info),
                                           Deoptimization::Reason_unloaded,
                                           Deoptimization::Action_make_not_entrant);
@@ -1901,8 +1902,9 @@
       (needs_patching ||
        MacroAssembler::needs_explicit_null_check(x->offset()) ||
        stress_deopt)) {
-    if (needs_patching && field_type == T_VALUETYPE) {
-      // We are loading a "Q" field, but the holder class is not yet loaded.
+    if (needs_patching && x->field()->signature()->starts_with("Q", 1)) {
+      // We are loading a field of type "QT;", but class T is not yet loaded. We don't know
+      // whether this field is flattened or not. Let's deoptimize and recompile.
       CodeStub* stub = new DeoptimizeStub(new CodeEmitInfo(info),
                                           Deoptimization::Reason_unloaded,
                                           Deoptimization::Action_make_not_entrant);
--- a/src/hotspot/share/ci/ciEnv.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/ci/ciEnv.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -391,8 +391,7 @@
 ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
                                        const constantPoolHandle& cpool,
                                        ciSymbol* name,
-                                       bool require_local,
-                                       bool is_value_type) {
+                                       bool require_local) {
   ASSERT_IN_VM;
   EXCEPTION_CONTEXT;
 
@@ -406,8 +405,7 @@
                     sym->utf8_length()-2,
                     KILL_COMPILE_ON_FATAL_(_unloaded_ciinstance_klass));
     ciSymbol* strippedname = get_symbol(strippedsym);
-    is_value_type = (sym->char_at(0) == 'Q');
-    return get_klass_by_name_impl(accessing_klass, cpool, strippedname, require_local, is_value_type);
+    return get_klass_by_name_impl(accessing_klass, cpool, strippedname, require_local);
   }
 
   // Check for prior unloaded klass.  The SystemDictionary's answers
@@ -462,12 +460,11 @@
                                                  KILL_COMPILE_ON_FATAL_(fail_type));
 
     // Get element ciKlass recursively.
-    is_value_type = (sym->char_at(1) == 'Q');
     ciKlass* elem_klass =
       get_klass_by_name_impl(accessing_klass,
                              cpool,
                              get_symbol(elem_sym),
-                             require_local, is_value_type);
+                             require_local);
     if (elem_klass != NULL && elem_klass->is_loaded()) {
       // Now make an array for it
       if (elem_klass->is_valuetype() && elem_klass->as_value_klass()->flatten_array()) {
@@ -505,23 +502,17 @@
     i++;
   }
   if (i > 0 && sym->char_at(i) == 'Q') {
-    if (EnableValhallaC1) {
-      return get_unloaded_klass(accessing_klass, name, true);
-    } else {
-      // FIXME - C2 can't handle unloaded ciValueKlass
-
-      // An unloaded array class of value types is an ObjArrayKlass, an
-      // unloaded value type class is an InstanceKlass. For consistency,
-      // make the signature of the unloaded array of value type use L
-      // rather than Q.
-      char *new_name = CURRENT_THREAD_ENV->name_buffer(sym->utf8_length()+1);
-      strncpy(new_name, (char*)sym->base(), sym->utf8_length());
-      new_name[i] = 'L';
-      new_name[sym->utf8_length()] = '\0';
-      return get_unloaded_klass(accessing_klass, ciSymbol::make(new_name), false);
-    }
+    // An unloaded array class of value types is an ObjArrayKlass, an
+    // unloaded value type class is an InstanceKlass. For consistency,
+    // make the signature of the unloaded array of value type use L
+    // rather than Q.
+    char *new_name = CURRENT_THREAD_ENV->name_buffer(sym->utf8_length()+1);
+    strncpy(new_name, (char*)sym->base(), sym->utf8_length());
+    new_name[i] = 'L';
+    new_name[sym->utf8_length()] = '\0';
+    return get_unloaded_klass(accessing_klass, ciSymbol::make(new_name));
   }
-  return get_unloaded_klass(accessing_klass, name, is_value_type);
+  return get_unloaded_klass(accessing_klass, name);
 }
 
 // ------------------------------------------------------------------
--- a/src/hotspot/share/ci/ciEnv.hpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/ci/ciEnv.hpp	Wed Mar 06 21:30:53 2019 -0800
@@ -140,8 +140,7 @@
   ciKlass* get_klass_by_name_impl(ciKlass* accessing_klass,
                                   const constantPoolHandle& cpool,
                                   ciSymbol* klass_name,
-                                  bool require_local,
-                                  bool is_value_type = false);
+                                  bool require_local);
   ciKlass*   get_klass_by_index_impl(const constantPoolHandle& cpool,
                                      int klass_index,
                                      bool& is_accessible,
@@ -249,8 +248,8 @@
   // Get a ciKlass representing an unloaded klass.
   // Ensures uniqueness of the result.
   ciKlass* get_unloaded_klass(ciKlass*  accessing_klass,
-                              ciSymbol* name, bool is_value_type = false) {
-    return _factory->get_unloaded_klass(accessing_klass, name, true, is_value_type);
+                              ciSymbol* name) {
+    return _factory->get_unloaded_klass(accessing_klass, name, true);
   }
 
   // Get a ciKlass representing an unloaded klass mirror.
--- a/src/hotspot/share/ci/ciObjectFactory.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/ci/ciObjectFactory.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -468,8 +468,7 @@
 // unloaded klass.  This may need to change.
 ciKlass* ciObjectFactory::get_unloaded_klass(ciKlass* accessing_klass,
                                              ciSymbol* name,
-                                             bool create_if_not_found,
-                                             bool is_value_type) {
+                                             bool create_if_not_found) {
   EXCEPTION_CONTEXT;
   oop loader = NULL;
   oop domain = NULL;
@@ -523,12 +522,7 @@
       // The element klass is a TypeArrayKlass.
       element_klass = ciTypeArrayKlass::make(element_type);
     }
-    if (element_type == T_VALUETYPE && EnableValhallaC1) { // FIXME C2 can't handle unloaded ciValueArrayKlass
-      assert(element_klass->is_valuetype(), "must be");
-      new_klass = new (arena()) ciValueArrayKlass(name, element_klass->as_value_klass(), dimension);
-    } else {
-      new_klass = new (arena()) ciObjArrayKlass(name, element_klass, dimension);
-    }
+    new_klass = new (arena()) ciObjArrayKlass(name, element_klass, dimension);
   } else {
     jobject loader_handle = NULL;
     jobject domain_handle = NULL;
@@ -536,11 +530,7 @@
       loader_handle = accessing_klass->loader_handle();
       domain_handle = accessing_klass->protection_domain_handle();
     }
-    if (is_value_type && EnableValhallaC1) { // FIXME C2 can't handle unloaded ciValueKlass
-      new_klass = new (arena()) ciValueKlass(name, loader_handle, domain_handle);
-    } else {
-      new_klass = new (arena()) ciInstanceKlass(name, loader_handle, domain_handle);
-    }
+    new_klass = new (arena()) ciInstanceKlass(name, loader_handle, domain_handle);
   }
   init_ident_of(new_klass);
   _unloaded_klasses->append(new_klass);
--- a/src/hotspot/share/ci/ciObjectFactory.hpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/ci/ciObjectFactory.hpp	Wed Mar 06 21:30:53 2019 -0800
@@ -115,8 +115,7 @@
   // Get a ciKlass representing an unloaded klass.
   ciKlass* get_unloaded_klass(ciKlass* accessing_klass,
                               ciSymbol* name,
-                              bool create_if_not_found,
-                              bool is_value_type = false);
+                              bool create_if_not_found);
 
   // Get a ciInstance representing an unresolved klass mirror.
   ciInstance* get_unloaded_klass_mirror(ciKlass* type);
--- a/src/hotspot/share/ci/ciValueArrayKlass.cpp	Sun Mar 03 22:09:42 2019 -0800
+++ b/src/hotspot/share/ci/ciValueArrayKlass.cpp	Wed Mar 06 21:30:53 2019 -0800
@@ -142,7 +142,8 @@
 ciValueArrayKlass* ciValueArrayKlass::make_impl(ciKlass* element_klass) {
   assert(ValueArrayFlatten, "should only be used for flattened value type arrays");
   assert(element_klass->is_valuetype(), "element type must be value type");
-  if (element_klass->is_loaded()) {
+  assert(element_klass->is_loaded(), "unloaded Q klasses are represented by ciInstanceKlass");
+  {
     EXCEPTION_CONTEXT;
     // The element klass is loaded
     Klass* array = element_klass->get_Klass()->array_klass(THREAD);
@@ -155,17 +156,6 @@
     }
     return CURRENT_THREAD_ENV->get_value_array_klass(array);
   }
-
-  // The array klass was unable to be made or the element klass was
-  // not loaded.
-  ciSymbol* array_name = construct_array_name(element_klass->name(), 1);
-  if (array_name == ciEnv::unloaded_cisymbol()) {
-    //return ciEnv::unloaded_ciobjarrayklass();
-    assert(0, "FIXME");
-  }
-  return
-    CURRENT_ENV->get_unloaded_klass(element_klass, array_name, false)
-                        ->as_value_array_klass();
 }
 
 // ------------------------------------------------------------------
--- a/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestUnloadedValueTypeArray.java	Sun Mar 03 22:09:42 2019 -0800
+++ b/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestUnloadedValueTypeArray.java	Wed Mar 06 21:30:53 2019 -0800
@@ -32,6 +32,8 @@
  *        -XX:CompileCommand=compileonly,TestUnloadedValueTypeArray::test2
  *        -XX:CompileCommand=compileonly,TestUnloadedValueTypeArray::test3
  *        -XX:CompileCommand=compileonly,TestUnloadedValueTypeArray::test4
+ *        -XX:CompileCommand=compileonly,TestUnloadedValueTypeArray::test5
+ *        -XX:CompileCommand=compileonly,TestUnloadedValueTypeArray::test6
  *      TestUnloadedValueTypeArray
  */
 
@@ -78,6 +80,28 @@
     }
 }
 
+value final class MyValue5 {
+    final int foo;
+
+    private MyValue5() {
+        foo = 0x53;
+    }
+    static MyValue5 make(int n) {
+        return __WithField(MyValue5.default.foo, n);
+    }
+}
+
+value final class MyValue6 {
+    final int foo = 0;
+
+    static MyValue6 make(int n) {
+        return __WithField(MyValue6.default.foo, n);
+    }
+    static MyValue6 make2(MyValue6 v, MyValue6[] dummy) {
+        return __WithField(v.foo, v.foo+1);
+    }
+}
+
 public class TestUnloadedValueTypeArray {
 
     static MyValue[] target() {
@@ -159,10 +183,58 @@
         Asserts.assertEQ(arr[1].foo, 2345);
     }
 
+    static Object[] test5(int n) {
+        if (n == 0) {
+            return null;
+        } else if (n == 1) {
+            MyValue5[] arr = new MyValue5[10];
+            arr[1] = MyValue5.make(12345);
+            return arr;
+        } else {
+            MyValue5.box[] arr = new MyValue5.box[10];
+            arr[1] = MyValue5.make(22345);
+            return arr;
+        }
+    }
+
+    static void test5_verifier() {
+        int n = 50000;
+
+        for (int i=0; i<n; i++) {
+            test5(0);
+        }
+
+        {
+            MyValue5[] arr = null;
+            for (int i=0; i<n; i++) {
+                arr = (MyValue5[])test5(1);
+            }
+            Asserts.assertEQ(arr[1].foo, 12345);
+        }
+        {
+            MyValue5.box[] arr = null;
+            for (int i=0; i<n; i++) {
+                arr = (MyValue5.box[])test5(2);
+            }
+            Asserts.assertEQ(arr[1].foo, 22345);
+        }
+    }
+
+    static Object test6() {
+        return MyValue6.make2(MyValue6.make(123), null);
+    }
+
+    static void test6_verifier() {
+        Object n = test6();
+        Asserts.assertEQ(n.toString(), "[MyValue6 foo=124]");
+    }
+
     static public void main(String[] args) {
         test1();
         test2_verifier();
         test3_verifier();
         test4_verifier();
+        test5_verifier();
+        test6_verifier();
     }
 }