RFR(XL): 6312651: Compiler should only use verified interface types for optimization

Roland Westrelin rwestrel at redhat.com
Mon May 20 11:42:21 UTC 2019


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