RFR(XL): 6312651: Compiler should only use verified interface types for optimization
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed Jun 26 16:05:15 UTC 2019
Hi Roland,
I started looking into the patch.
Instead of doing high-level review first, I rebased the patch to jdk/jdk
tip and initial round of testing spotted the following problems:
(1) build failure on SPARC [1]
The following workaround solved the problem:
diff --git a/src/hotspot/share/opto/type.hpp
b/src/hotspot/share/opto/type.hpp
--- a/src/hotspot/share/opto/type.hpp
+++ b/src/hotspot/share/opto/type.hpp
@@ -804,7 +804,6 @@
class InterfaceSet {
private:
GrowableArray<ciKlass*> _list;
- static int compare(ciKlass* const &, ciKlass* const & k2);
void raw_add(ciKlass* interface);
void verify() const;
public:
@@ -818,6 +817,8 @@
bool empty() const { return _list.length() == 0; }
GrowableArray<ciKlass*>* list() const;
+ static int compare(ciKlass* const &, ciKlass* const & k2);
+
inline void* operator new( size_t x ) throw() {
Compile* compile = Compile::current();
return compile->type_arena()->Amalloc_D(x);
============
(2) Numerous test failures:
# Internal Error (src/hotspot/share/opto/type.cpp:3696), pid=75399,
tid=43267
# assert(!ik->is_interface()) failed: no interface here
The problem is triggered by array of interface types.
Used the following workaround:
diff --git a/src/hotspot/share/opto/type.cpp
b/src/hotspot/share/opto/type.cpp
--- a/src/hotspot/share/opto/type.cpp
+++ b/src/hotspot/share/opto/type.cpp
@@ -3385,6 +3385,9 @@
assert(interfaces.eq(*TypeAryPtr::_array_interfaces), "unexpected
array interfaces");
// Element is an object array. Recursively call ourself.
ciKlass* eklass = klass->as_obj_array_klass()->element_klass();
+ if (eklass->is_loaded() && eklass->is_interface()) {
+ eklass = ciEnv::current()->Object_klass();
+ }
const TypeOopPtr *etype =
TypeOopPtr::make_from_klass_common(eklass, TypePtr::interfaces(eklass,
true, true, true), false, try_for_exact);
bool xk = etype->klass_is_exact();
const TypeAry* arr0 = TypeAry::make(etype, TypeInt::POS);
============
(3) Unloaded classes in signatures trigger the following assert:
# Internal Error (open/src/hotspot/share/ci/ciInstanceKlass.hpp:145),
pid=6557, tid=6570
# assert(is_loaded()) failed: must be loaded
V [libjvm.so+0x5ff406] ciInstanceKlass::is_interface()+0x56
V [libjvm.so+0x17049ad] TypeKlassPtr::TypeKlassPtr(Type::TYPES,
TypePtr::PTR, ciKlass*, TypePtr::InterfaceSet const&, int)+0xfd
V [libjvm.so+0x1712d9b]
TypeInstKlassPtr::TypeInstKlassPtr(TypePtr::PTR, ciKlass*,
TypePtr::InterfaceSet const&, int)+0x2b
V [libjvm.so+0x1708284] TypeInstPtr::as_klass_type(bool) const+0xa4
V [libjvm.so+0x14441b7] Parse::check_interpreter_type(Node*, Type
const*, SafePointNode*&)+0x467
V [libjvm.so+0x1444c51] Parse::load_interpreter_state(Node*)+0x971
V [libjvm.so+0x14457bd] Parse::Parse(JVMState*, ciMethod*, float)+0x60d
Used workaround:
diff --git a/src/hotspot/share/opto/parse1.cpp
b/src/hotspot/share/opto/parse1.cpp
--- a/src/hotspot/share/opto/parse1.cpp
+++ b/src/hotspot/share/opto/parse1.cpp
@@ -167,7 +167,7 @@
// When paths are cut off, values at later merge points can rise
// toward more specific classes. Make sure these specific classes
// are still in effect.
- if (tp != NULL) {
+ if (tp != NULL && tp->is_loaded()) {
// TypeFlow asserted a specific object type. Value must have that
type.
Node* bad_type_ctrl = NULL;
l = gen_checkcast(l,
makecon(tp->as_klass_type()->cast_to_exactness(true)), &bad_type_ctrl);
============
(4) runtime/Unsafe/AllocateInstance.java
java.lang.AssertionError: Should throw InstantiationException for an
interface
at AllocateInstance.testInterface(AllocateInstance.java:71)
at AllocateInstance.main(AllocateInstance.java:81)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
at java.base/java.lang.Thread.run(Thread.java:830)
Initial problem is EA eliminates non-escaping allocation of an interface
class (ik == Object vs cik == InterfaceClass):
src/hotspot/share/opto/escape.cpp
@@ -819,11 +819,12 @@
}
}
} else { // Allocate instance
- ciInstanceKlass* ik = kt->is_instklassptr()->instance_klass();
- if (ik->is_subclass_of(_compile->env()->Thread_klass()) ||
- ik->is_subclass_of(_compile->env()->Reference_klass()) ||
- !ik->can_be_instantiated() ||
- ik->has_finalizer()) {
es = PointsToNode::GlobalEscape;
}
}
Then, Allocate expansion uses wrong instance layout info (again Object
vs InterfaceClass):
src/hotspot/share/opto/graphKit.cpp
@@ -3470,7 +3470,7 @@
}
lhelper = Klass::array_layout_helper(elem);
} else {
- lhelper =
inst_klass->is_instklassptr()->instance_klass()->layout_helper();
}
if (lhelper != Klass::_lh_neutral_value) {
constant_value = lhelper;
I'll wait for the fixes before initiating broader testing.
Best regards,
Vladimir Ivanov
[1]
src/hotspot/share/opto/type.cpp, line 2977: Error: static
TypePtr::InterfaceSet::compare(ciKlass*const&, ciKlass*const&) is not
accessible from ciKlass*
GrowableArray<ciKlass*>::insert_sorted<&TypePtr::InterfaceSet::compare>(ciKlass*const&).
src/hotspot/share/opto/type.cpp, line 2977: Where, temwhileinst:
While instantiating "ciKlass*
GrowableArray<ciKlass*>::insert_sorted<&TypePtr::InterfaceSet::compare>(ciKlass*const&)".
src/hotspot/share/opto/type.cpp, line 2977: Where, teminstend:
Instantiated from non-template code.
src/hotspot/share/opto/type.cpp, line 2977: Error: static
TypePtr::InterfaceSet::compare(ciKlass*const&, ciKlass*const&) is not
accessible from ciKlass*
GrowableArray<ciKlass*>::insert_sorted<&TypePtr::InterfaceSet::compare>(ciKlass*const&).
src/hotspot/share/opto/type.cpp, line 2977: Where, temwhileinst:
While instantiating "ciKlass*
GrowableArray<ciKlass*>::insert_sorted<&TypePtr::InterfaceSet::compare>(ciKlass*const&)".
src/hotspot/share/opto/type.cpp, line 2977: Where, teminstend:
Instantiated from non-template code.
On 20/05/2019 14:42, Roland Westrelin wrote:
>
> http://cr.openjdk.java.net/~roland/6312651/webrev.00/
>
> This is a fix for a bug that John filed 14 years ago (almost) but has
> always affected c2 as far as I understand. The fix is implemented along
> the lines of John's recommendation: 1) the type system should keep track
> of an object's instance class and the set of interfaces it implements
> separately (or array of instance klass + set of interfaces), 2)
> interfaces in signatures should not be trusted.
>
> For 1), the _klass in TypeOopPtr/TypeKlassPtr now only refers to
> instance classes (or arrays of instance class). TypeOopPtr/TypeKlassPtr
> has an extra field _interfaces that carries the set of interfaces
> implemented by that type.
>
> For klass pointers, TypeKlassPtr is no longer sufficient because for
> arrays, we need the type to also include interfaces that the element
> implements. So I added 2 classes to the type system, TypeKlassInstPtr
> and TypeAryKlassPtr that mirror TypeInstPtr/TypeAryPtr. TypeAryKlassPtr
> has a TypeKlassPtr element which can then record what interfaces the
> element type implements. The meet implementation for both also mirrors
> TypeInstPtr/TypeAryPtr closely.
>
> In the existing implementation the TypeOopPtr/TypeKlassPtr klass()
> accessor is used in a lot of places to perform class equality checks,
> subtyping tests or to build a TypeOopPtr from a TypeKlassPtr or the
> other way around. That can't work now because klass() only gives partial
> information about the type. Rather than expose _klass and _interfaces
> through accessors, I have:
>
> - made the klass() accessor non public so code that's not closely tied
> to the actual type system implementation shouldn't use it
>
> - made some operations go through the type classes: testing for class
> equality should be done through klass_eq() which tests both class and
> interface set equality, same goes for subtyping tests. Creating a
> TypeKlassPtr from a TypeOopPtr or the other way around is now always
> done with the as_klass_type()/as_instance_type() methods.
>
> - introduced 3 new accessors: instance_klass() on
> TypeInstPtr/TypeInstKlassPtr which only returns the instance class,
> ignoring any interface implemented by the type; exact_klass() on
> TypeOopPtr/TypeKlassPtr which only works for exact types and can
> return an interface or array of interfaces; base_element_type() on
> TypeAryPtr/TypeAryKlassPtr() which returns the base element of the
> array and in case of an array of objects, only the instance class, not
> whatever interface the element implements. These 3 are AFAICT good
> enough to support existing c2 optimizations.
>
> For 2), TypeOopPtr::cast_to_non_interface() is used to filter out
> interfaces whenever signatures are used.
>
> As expected, this change makes several workarounds no longer necessary
> and cleans up the code quite a bit. For instance,
> CheckCastPPNode::Value() now falls back to the "right" implementation of
> ConstraintCastNode::Value() which leverages the type system.
>
> Fixing CheckCastPPNode::Value() exposes a bug in the C2 type checking
> logic: if a type check is proved to always fail during optimizations,
> the CheckCastPPNode becomes top, but the actual type checking logic in
> Phase::gen_subtype_check() doesn't always optimize out so the data path
> dies but not the control path. To fix that, I added a
> PartialSubtypeCheckNode::Value() method that triggers on subtype check
> failures consistently with CheckCastPPNode::Value. Problem is
> PartialSubtypeCheckNode is not checked first in
> Phase::gen_subtype_check(). So I added a duplicate test on the
> PartialSubtypeCheckNode result first in that logic, under an Opaque4Node
> so the actual is never compiled in the final code.
>
> Note that "8220416: Comparison of klass pointers is not optimized any
> more" is fixed by this.
>
> CastNullCheckDroppingsTest.java needs a change because
> testObjClassCast() has a call to objClass.cast() followed by a cast to
> String, so 2 casts in a raw once compiled. With the existing
> implementation, c2 can't see the casts as redundant and it keeps the
> second one. That one triggers a deopt if the test is passed null. The
> Class::cast implementation nevers traps for null. With the updated
> implementation, c2 sees the second cast as useless and optimizes it out.
>
> I dropped the CmpNNode::sub() becasue CmpN nodes are only created during
> final graph reshape so this is dead code.
>
> Performance is unaffected by this change AFAICT>
>
> Roland.
>
More information about the hotspot-compiler-dev
mailing list