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

[Vladimir Ivanov]

Hi Roland,

I'm very interested in the enhancements you propose and I plan to look 
into it, but I need more time to finish the review.

I hope hope you are fine with deferring it for jdk14.

Best regards,
Vladimir Ivanov

On 12/06/2019 18:14, Roland Westrelin wrote:
> 
> 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.