RFR: 8297933: [REDO] Compiler should only use verified interface types for optimization [v2]

[Vladimir Kozlov]

On Wed, 14 Dec 2022 13:50:47 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This PR re-does 6312651 (Compiler should only use verified interface
>> types for optimization) with a couple fixes I had pushed afterward
>> (8297556 and 8297343) and fixes for some other issues.
>> 
>> The trickiest one is a fix for 8297345 (C2: SIGSEGV in
>> PhaseIdealLoop::push_pinned_nodes_thru_region) for which I added a
>> test case. The crash occurs because a (If (Bool (CmpP (LoadKlass ..))))
>> only has a single projection. It lost the other projection because of
>> a CheckCastPP that becomes top. Initially the pattern is, in pseudo
>> code,:
>> 
>> if (obj.klass == some_class) {
>>   obj = CheckCastPP#1(obj);
>> }
>> 
>> obj itself is a CheckCastPP that's pinned at a dominating if. That
>> dominating if goes through split through phi. The LoadKlass for the
>> pseudo code above also has control set to the dominating if being
>> transformed. This result in:
>> 
>> if (phi1 == some_class) {
>>   obj = CheckCastPP#1(phi2);
>> }
>> 
>> with phi1 = (Phi (LoadKlass obj) (LoadKlass obj)) and phi2 = (Phi obj obj)
>> with obj = (CheckCastPP#2 obj')
>> 
>> PhiNode::Ideal() transforms phi2 into a new CheckCastPP:
>> (CheckCastPP#3 obj' obj') with control set to the region right above
>> the if in the pseudo code above. There happens to be another
>> CheckCastPP at the same control which casts obj' to a narrower
>> type. So the new CheckCastPP#3 is replaced by that one (because of
>> ConstraintCastNode::dominating_cast())and pseudo code becomes:
>> 
>> if (phi1 == some_class) {
>>   obj = CheckCastPP#1(CheckCastPP#4(obj'));
>> }
>> 
>> and then:
>> 
>> if (phi1 == some_class) {
>>   obj = top;
>> }
>> 
>> because the types of the 2 CheckCastPPs conflict. That would be ok if:
>> 
>> phi1 == some_class
>> 
>> would constant fold. It would if the test was:
>> 
>> if (CheckCastPP#4(obj').klass == some_klass) {
>> 
>> but because of split if, the (CmpP (LoadKlass ..)) and the
>> CheckCastPP#1 ended up with 2 different object inputs that then were
>> transformed differently. The fix I propose is to have split if clone the entire:
>> 
>> (Bool (CmpP (LoadKlass (AddP ..))))
>> 
>> down the same way (Bool (CmpP ..)) is cloned down. After split if, the
>> pseudo code becomes:
>> 
>> if (phi.klass == some_class) {
>>   obj = CheckCastPP#1(phi);
>> }
>> 
>> The bug can't occur because the CheckCastPP and (CmpP (LoadKlass ..))
>> operate on the same phi input. The change in split_if.cpp implements
>> that.
>> 
>> The other fixes are:
>> 
>> - arraycopynode.cpp: a crash happens because dest_offset and
>>   src_offset are the same. The call to transform that results in
>>   src_scale, causes src_offset (and thus dest_offset) to become
>>   dead. The fix is to add a hook node to preserve dest_offset. This is
>>   unrelated to 6312651 but it triggers with that change for some
>>   reason.
>>   
>> - castnode.cpp: I removed CheckCastPPNode::Identity(), a piece of code
>>   that the change in the handling of interfaces make obsolete and that
>>   I missed in the PR for 6312651.
>>   
>> - castnode.cpp: the change in CheckCastPPNode::Value() fixes a rare
>>   assert when during CCP, Value() is called with an input raw constant
>>   ptr.
>>   
>> - type.cpp: a _klass = NULL field in arrays used to indicate only top
>>   or bottom but I changed that so _klass is only guaranteed non null
>>   for basic type arrays. The fix in type.cpp updates a piece of code
>>   that I didn't adapt to the new meaning of _klass = NULL.
>>   
>> - the other changes are due to StressReflectiveCode. With 6312651, a
>>   CheckCastPP can fold to top if it sees a type for its input that
>>   conflicts with its own type. That wasn't the case before. So if a
>>   type check fails, a CheckCastPP will fold to top and the control
>>   flow branch it's in must die. That doesn't always happen with
>>   StressReflectiveCode: the CheckCastPP folds but not the control flow
>>   path. With ExpandSubTypeCheckAtParseTime on, that's because of a
>>   code path in LoadNode::Value() that's disabled with
>>   StressReflectiveCode. With ExpandSubTypeCheckAtParseTime off, it's
>>   because Compile::static_subtype_check() is always pessimistic with
>>   StressReflectiveCode but it's used by SubTypeCheckNode::sub() to
>>   find when a node can constant fold.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   test fix

@rwestrel Can you push split-if/CheckCastPP fix separately, before these changes?
It is not directly related to this work.

src/hotspot/share/ci/ciInstanceKlass.cpp line 745:

> 743:           Array<InstanceKlass*>* interfaces = ik->transitive_interfaces();
> 744:           Arena* arena = CURRENT_ENV->arena();
> 745:           int len = interfaces->length() + (is_interface() ? 1 : 0);

I think you need to cache `interfaces->length()` in local variable to use in following loop to make sure it is the same value. I concern that an other thread may change it.

-------------

PR: https://git.openjdk.org/jdk/pull/11666