diff --git a/src/hotspot/share/ci/ciArray.cpp b/src/hotspot/share/ci/ciArray.cpp index 7d7a3ddea94..6f601c87ff2 100644 --- a/src/hotspot/share/ci/ciArray.cpp +++ b/src/hotspot/share/ci/ciArray.cpp @@ -63,9 +63,7 @@ ciConstant ciArray::element_value_impl(BasicType elembt, assert(ary->is_objArray(), ""); objArrayOop objary = (objArrayOop) ary; oop elem = objary->obj_at(index); - ciEnv* env = CURRENT_ENV; - ciObject* box = env->get_object(elem); - return ciConstant(T_OBJECT, box); + return ciConstant(elembt, CURRENT_ENV->get_object(elem)); } default: break; @@ -94,9 +92,15 @@ ciConstant ciArray::element_value_impl(BasicType elembt, // Returns T_ILLEGAL if there is no element at the given index. ciConstant ciArray::element_value(int index) { BasicType elembt = element_basic_type(); + ciConstant value = check_constant_value_cache(index, elembt); + if (value.is_valid()) { + return value; + } GUARDED_VM_ENTRY( - return element_value_impl(elembt, get_arrayOop(), index); + value = element_value_impl(elembt, get_arrayOop(), index); ) + add_to_constant_value_cache(index, value); + return value; } // ------------------------------------------------------------------ diff --git a/src/hotspot/share/ci/ciConstant.cpp b/src/hotspot/share/ci/ciConstant.cpp index 2e34ba02fed..62ff0ab08c9 100644 --- a/src/hotspot/share/ci/ciConstant.cpp +++ b/src/hotspot/share/ci/ciConstant.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2023, 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 @@ -32,6 +32,35 @@ // // This class represents a constant value. +// ------------------------------------------------------------------ +// ciConstant::is_null_or_zero +bool ciConstant::is_null_or_zero() const { + if (!is_java_primitive(basic_type())) { + return as_object()->is_null_object(); + } else if (type2size[basic_type()] == 1) { + // treat float bits as int, to avoid comparison with -0 and NaN + return (_value._int == 0); + } else if (type2size[basic_type()] == 2) { + // treat double bits as long, to avoid comparison with -0 and NaN + return (_value._long == 0); + } else { + return false; + } +} + +// ------------------------------------------------------------------ +// ciConstant::is_loaded +bool ciConstant::is_loaded() const { + if (is_valid()) { + if (is_reference_type(basic_type())) { + return as_object()->is_loaded(); + } else { + return true; + } + } + return false; +} + // ------------------------------------------------------------------ // ciConstant::print void ciConstant::print() { diff --git a/src/hotspot/share/ci/ciConstant.hpp b/src/hotspot/share/ci/ciConstant.hpp index 25591699045..1d0f60407ed 100644 --- a/src/hotspot/share/ci/ciConstant.hpp +++ b/src/hotspot/share/ci/ciConstant.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2023, 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 @@ -26,7 +26,7 @@ #define SHARE_CI_CICONSTANT_HPP #include "ci/ciClassList.hpp" -#include "ci/ciNullObject.hpp" +#include "utilities/globalDefinitions.hpp" // ciConstant // @@ -110,34 +110,14 @@ public: return _value._object; } - bool is_null_or_zero() const { - if (!is_java_primitive(basic_type())) { - return as_object()->is_null_object(); - } else if (type2size[basic_type()] == 1) { - // treat float bits as int, to avoid comparison with -0 and NaN - return (_value._int == 0); - } else if (type2size[basic_type()] == 2) { - // treat double bits as long, to avoid comparison with -0 and NaN - return (_value._long == 0); - } else { - return false; - } - } + bool is_null_or_zero() const; bool is_valid() const { return basic_type() != T_ILLEGAL; } - bool is_loaded() const { - if (is_valid()) { - if (is_reference_type(basic_type())) { - return as_object()->is_loaded(); - } else { - return true; - } - } - return false; - } + bool is_loaded() const; + // Debugging output void print(); }; diff --git a/src/hotspot/share/ci/ciField.cpp b/src/hotspot/share/ci/ciField.cpp index 9fbe4af64bd..6245e0aafb9 100644 --- a/src/hotspot/share/ci/ciField.cpp +++ b/src/hotspot/share/ci/ciField.cpp @@ -311,8 +311,7 @@ ciConstant ciField::constant_value() { } if (_constant_value.basic_type() == T_ILLEGAL) { // Static fields are placed in mirror objects. - VM_ENTRY_MARK; - ciInstance* mirror = CURRENT_ENV->get_instance(_holder->get_Klass()->java_mirror()); + ciInstance* mirror = _holder->java_mirror(); _constant_value = mirror->field_value_impl(type()->basic_type(), offset()); } if (FoldStableValues && is_stable() && _constant_value.is_null_or_zero()) { diff --git a/src/hotspot/share/ci/ciInstance.cpp b/src/hotspot/share/ci/ciInstance.cpp index 66175623665..0c8be93162c 100644 --- a/src/hotspot/share/ci/ciInstance.cpp +++ b/src/hotspot/share/ci/ciInstance.cpp @@ -28,6 +28,7 @@ #include "ci/ciField.hpp" #include "ci/ciInstance.hpp" #include "ci/ciInstanceKlass.hpp" +#include "ci/ciNullObject.hpp" #include "ci/ciUtilities.inline.hpp" #include "classfile/vmClasses.hpp" #include "oops/oop.inline.hpp" @@ -59,17 +60,22 @@ ciType* ciInstance::java_mirror_type() { // ------------------------------------------------------------------ // ciInstance::field_value_impl ciConstant ciInstance::field_value_impl(BasicType field_btype, int offset) { + ciConstant value = check_constant_value_cache(offset, field_btype); + if (value.is_valid()) { + return value; + } + VM_ENTRY_MARK; oop obj = get_oop(); assert(obj != nullptr, "bad oop"); switch(field_btype) { - case T_BYTE: return ciConstant(field_btype, obj->byte_field(offset)); - case T_CHAR: return ciConstant(field_btype, obj->char_field(offset)); - case T_SHORT: return ciConstant(field_btype, obj->short_field(offset)); - case T_BOOLEAN: return ciConstant(field_btype, obj->bool_field(offset)); - case T_INT: return ciConstant(field_btype, obj->int_field(offset)); - case T_FLOAT: return ciConstant(obj->float_field(offset)); - case T_DOUBLE: return ciConstant(obj->double_field(offset)); - case T_LONG: return ciConstant(obj->long_field(offset)); + case T_BYTE: value = ciConstant(field_btype, obj->byte_field(offset)); break; + case T_CHAR: value = ciConstant(field_btype, obj->char_field(offset)); break; + case T_SHORT: value = ciConstant(field_btype, obj->short_field(offset)); break; + case T_BOOLEAN: value = ciConstant(field_btype, obj->bool_field(offset)); break; + case T_INT: value = ciConstant(field_btype, obj->int_field(offset)); break; + case T_FLOAT: value = ciConstant(obj->float_field(offset)); break; + case T_DOUBLE: value = ciConstant(obj->double_field(offset)); break; + case T_LONG: value = ciConstant(obj->long_field(offset)); break; case T_OBJECT: // fall through case T_ARRAY: { oop o = obj->obj_field(offset); @@ -82,15 +88,17 @@ ciConstant ciInstance::field_value_impl(BasicType field_btype, int offset) { // information about the object's class (which is exact) or length. if (o == nullptr) { - return ciConstant(field_btype, ciNullObject::make()); + value = ciConstant(field_btype, ciNullObject::make()); } else { - return ciConstant(field_btype, CURRENT_ENV->get_object(o)); + value = ciConstant(field_btype, CURRENT_ENV->get_object(o)); } + break; } default: fatal("no field value: %s", type2name(field_btype)); - return ciConstant(); } + add_to_constant_value_cache(offset, value); + return value; } // ------------------------------------------------------------------ @@ -101,8 +109,7 @@ ciConstant ciInstance::field_value(ciField* field) { assert(is_loaded(), "invalid access - must be loaded"); assert(field->holder()->is_loaded(), "invalid access - holder must be loaded"); assert(field->is_static() || klass()->is_subclass_of(field->holder()), "invalid access - must be subclass"); - - GUARDED_VM_ENTRY(return field_value_impl(field->type()->basic_type(), field->offset());) + return field_value_impl(field->type()->basic_type(), field->offset()); } // ------------------------------------------------------------------ diff --git a/src/hotspot/share/ci/ciObject.cpp b/src/hotspot/share/ci/ciObject.cpp index e2d7dddb6f2..564443fe1d3 100644 --- a/src/hotspot/share/ci/ciObject.cpp +++ b/src/hotspot/share/ci/ciObject.cpp @@ -168,6 +168,38 @@ jobject ciObject::constant_encoding() { return handle(); } +// ------------------------------------------------------------------ +// ciObject::check_constant_value_cache() +// +// Cache constant value lookups to ensure that consistent values are observed +// during compilation because fields may be (re-)initialized concurrently. +ciConstant ciObject::check_constant_value_cache(int off, BasicType bt) { + if (_constant_values != nullptr) { + for (int i = 0; i < _constant_values->length(); ++i) { + ConstantValue cached_val = _constant_values->at(i); + if (cached_val.off() == off) { + assert(cached_val.value().basic_type() == bt, "unexpected type"); + return cached_val.value(); + } + } + } + return ciConstant(); +} + +// ------------------------------------------------------------------ +// ciObject::add_to_constant_value_cache() +// +// Add a constant value to the cache. +void ciObject::add_to_constant_value_cache(int off, ciConstant val) { + assert(val.is_valid(), "value must be valid"); + assert(!check_constant_value_cache(off, val.basic_type()).is_valid(), "duplicate"); + if (_constant_values == nullptr) { + Arena* arena = CURRENT_ENV->arena(); + _constant_values = new (arena) GrowableArray(arena, 1, 0, ConstantValue()); + } + _constant_values->append(ConstantValue(off, val)); +} + // ------------------------------------------------------------------ // ciObject::should_be_constant() bool ciObject::should_be_constant() { diff --git a/src/hotspot/share/ci/ciObject.hpp b/src/hotspot/share/ci/ciObject.hpp index 213e147e2d3..af80dfcf6b4 100644 --- a/src/hotspot/share/ci/ciObject.hpp +++ b/src/hotspot/share/ci/ciObject.hpp @@ -27,7 +27,9 @@ #include "ci/ciBaseObject.hpp" #include "ci/ciClassList.hpp" +#include "ci/ciConstant.hpp" #include "runtime/handles.hpp" +#include "utilities/growableArray.hpp" // ciObject // @@ -57,6 +59,22 @@ private: jobject _handle; ciKlass* _klass; + // Cache constant value lookups to ensure that consistent values are observed during compilation. + class ConstantValue { + private: + ciConstant _value; + int _off; + + public: + ConstantValue() : _value(ciConstant()), _off(0) { } + ConstantValue(int off, ciConstant value) : _value(value), _off(off) { } + + int off() const { return _off; } + ciConstant value() const { return _value; } + }; + + GrowableArray* _constant_values = nullptr; + protected: ciObject(); ciObject(oop o); @@ -94,6 +112,10 @@ public: // be registered with the oopRecorder. jobject constant_encoding(); + // Access to the constant value cache + ciConstant check_constant_value_cache(int off, BasicType bt); + void add_to_constant_value_cache(int off, ciConstant val); + virtual bool is_object() const { return true; } // What kind of ciObject is this? diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 93ff6e81e35..cb4b13867ee 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2023, 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 @@ -1759,19 +1759,15 @@ PhaseCCP::~PhaseCCP() { #ifdef ASSERT -static bool ccp_type_widens(const Type* t, const Type* t0) { - assert(t->meet(t0) == t->remove_speculative(), "Not monotonic"); - switch (t->base() == t0->base() ? t->base() : Type::Top) { - case Type::Int: - assert(t0->isa_int()->_widen <= t->isa_int()->_widen, "widen increases"); - break; - case Type::Long: - assert(t0->isa_long()->_widen <= t->isa_long()->_widen, "widen increases"); - break; - default: - break; +void PhaseCCP::verify_type(Node* n, const Type* tnew, const Type* told) { + if (tnew->meet(told) != tnew->remove_speculative()) { + n->dump(1); + tty->print("told = "); told->dump(); tty->cr(); + tty->print("tnew = "); tnew->dump(); tty->cr(); + fatal("Not monotonic"); } - return true; + assert(!told->isa_int() || !tnew->isa_int() || told->is_int()->_widen <= tnew->is_int()->_widen, "widen increases"); + assert(!told->isa_long() || !tnew->isa_long() || told->is_long()->_widen <= tnew->is_long()->_widen, "widen increases"); } #endif //ASSERT @@ -1806,7 +1802,7 @@ void PhaseCCP::analyze() { } const Type* new_type = n->Value(this); if (new_type != type(n)) { - assert(ccp_type_widens(new_type, type(n)), "ccp type must widen"); + DEBUG_ONLY(verify_type(n, new_type, type(n));) dump_type_and_node(n, new_type); set_type(n, new_type); push_child_nodes_to_worklist(worklist, n); @@ -1834,12 +1830,13 @@ void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) { continue; // ignore integer widen } } - if (n->is_Load()) { + if (n->is_Load() && !told->singleton()) { // MemNode::can_see_stored_value looks up through many memory nodes, // which means we would need to notify modifications from far up in // the inputs all the way down to the LoadNode. We don't do that. continue; } + verify_type(n, tnew, told); tty->cr(); tty->print_cr("Missed optimization (PhaseCCP):"); n->dump_bfs(1, 0, ""); diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index dc5847b4845..cba767e6ce1 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2023, 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 @@ -605,6 +605,7 @@ class PhaseCCP : public PhaseIterGVN { // Worklist algorithm identifies constants void analyze(); #ifdef ASSERT + void verify_type(Node* n, const Type* tnew, const Type* told); // For every node n on verify list, check if type(n) == n->Value() void verify_analyze(Unique_Node_List& worklist_verify); #endif diff --git a/src/hotspot/share/opto/type.cpp b/src/hotspot/share/opto/type.cpp index 884555870b9..08ebb8ceed9 100644 --- a/src/hotspot/share/opto/type.cpp +++ b/src/hotspot/share/opto/type.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2023, 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 @@ -3370,7 +3370,6 @@ TypeOopPtr::TypeOopPtr(TYPES t, PTR ptr, ciKlass* k, const InterfaceSet& interfa _offset != arrayOopDesc::length_offset_in_bytes()); } else if (klass()->is_instance_klass()) { ciInstanceKlass* ik = klass()->as_instance_klass(); - ciField* field = NULL; if (this->isa_klassptr()) { // Perm objects don't use compressed references } else if (_offset == OffsetBot || _offset == OffsetTop) { @@ -3402,7 +3401,7 @@ TypeOopPtr::TypeOopPtr(TYPES t, PTR ptr, ciKlass* k, const InterfaceSet& interfa } } else { // Instance fields which contains a compressed oop references. - field = ik->get_field_by_offset(_offset, false); + ciField* field = ik->get_field_by_offset(_offset, false); if (field != NULL) { BasicType basic_elem_type = field->layout_type(); _is_ptr_to_narrowoop = UseCompressedOops && ::is_reference_type(basic_elem_type); diff --git a/test/hotspot/jtreg/compiler/stable/TestUnstableStable.java b/test/hotspot/jtreg/compiler/stable/TestUnstableStable.java new file mode 100644 index 00000000000..140933daa34 --- /dev/null +++ b/test/hotspot/jtreg/compiler/stable/TestUnstableStable.java @@ -0,0 +1,165 @@ +/* + * Copyright (c) 2023, 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 + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8295486 + * @summary Verify that constant folding of field loads observes consistent values during compilation. + * @library /test/lib / + * @modules java.base/jdk.internal.vm.annotation + * java.base/jdk.internal.misc + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/bootclasspath/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI + * -XX:-TieredCompilation -Xbatch -XX:PerMethodRecompilationCutoff=-1 + * -XX:CompileCommand=compileonly,compiler.stable.TestUnstableStable::test* + * compiler.stable.TestUnstableStable + */ + +package compiler.stable; + +import compiler.whitebox.CompilerWhiteBoxTest; + +import java.lang.reflect.Method; + +import jdk.internal.misc.Unsafe; +import jdk.internal.vm.annotation.Stable; +import jdk.test.whitebox.WhiteBox; + +public class TestUnstableStable { + static final Unsafe U = Unsafe.getUnsafe(); + static final WhiteBox WHITE_BOX = WhiteBox.getWhiteBox(); + static final TestUnstableStable HOLDER = new TestUnstableStable(); + + @Stable Integer stableField = null; + static @Stable Integer staticStableField = null; + static @Stable Integer[] stableArray0 = new Integer[1]; + static @Stable Integer[][] stableArray1 = new Integer[1][1]; + + static final Integer finalField = 43; + + static final long FIELD_OFFSET; + static { + try { + FIELD_OFFSET = U.staticFieldOffset(TestUnstableStable.class.getDeclaredField("finalField")); + } catch (NoSuchFieldException e) { + throw new RuntimeException("Field not found", e); + } + } + + static class Writer implements Runnable { + public void run() { + while (true) { + HOLDER.stableField = null; + HOLDER.stableField = 42; + HOLDER.stableField = 43; + staticStableField = null; + staticStableField = 42; + staticStableField = 43; + stableArray0[0] = null; + stableArray0[0] = 42; + stableArray0[0] = 43; + stableArray1[0] = null; + Integer[] tmp1 = {null}; + stableArray1[0] = tmp1; + Integer[] tmp2 = {42}; + stableArray1[0] = tmp2; + Integer[] tmp3 = {43}; + stableArray1[0] = tmp3; + stableArray1[0][0] = null; + stableArray1[0][0] = 42; + stableArray1[0][0] = 43; + U.putObject(TestUnstableStable.class, FIELD_OFFSET, null); + U.putObject(TestUnstableStable.class, FIELD_OFFSET, 42); + U.putObject(TestUnstableStable.class, FIELD_OFFSET, 43); + } + } + } + + static Object testNonStatic() { + // Trigger PhaseCCP and LoadNode::Value -> Type::make_constant_from_field + // which may observe different values of the stable field when invoked twice. + Integer val = HOLDER.stableField; + if (val == null) { + val = null; + } + return val; + } + + static Object testStatic() { + Integer val = staticStableField; + if (val == null) { + val = null; + } + return val; + } + + static Object testArray0() { + Integer val = stableArray0[0]; + if (val == null) { + val = null; + } + return val; + } + + static Object testArray1() { + Integer[] val = stableArray1[0]; + if (val == null) { + val = null; + } else { + return val[0]; + } + return val; + } + + static Object testFinal() { + Integer val = finalField; + if (val == null) { + val = null; + } + return val; + } + + public static void main(String[] args) throws Exception { + Thread t = new Thread(new Writer()); + t.start(); + Method testNonStatic = TestUnstableStable.class.getDeclaredMethod("testNonStatic"); + Method testStatic = TestUnstableStable.class.getDeclaredMethod("testStatic"); + Method testArray0 = TestUnstableStable.class.getDeclaredMethod("testArray0"); + Method testArray1 = TestUnstableStable.class.getDeclaredMethod("testArray1"); + Method testFinal = TestUnstableStable.class.getDeclaredMethod("testFinal"); + testFinal(); + for (int i = 0; i < 1000; ++i) { + WHITE_BOX.deoptimizeMethod(testNonStatic, false); + WHITE_BOX.enqueueMethodForCompilation(testNonStatic, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION); + WHITE_BOX.deoptimizeMethod(testStatic, false); + WHITE_BOX.enqueueMethodForCompilation(testStatic, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION); + WHITE_BOX.deoptimizeMethod(testArray0, false); + WHITE_BOX.enqueueMethodForCompilation(testArray0, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION); + WHITE_BOX.deoptimizeMethod(testArray1, false); + WHITE_BOX.enqueueMethodForCompilation(testArray1, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION); + WHITE_BOX.deoptimizeMethod(testFinal, false); + WHITE_BOX.enqueueMethodForCompilation(testFinal, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION); + } + } +}