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

[Roland Westrelin]

Any taker for this?

Roland.

Roland Westrelin <rwestrel at redhat.com> writes:

> 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.