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

[Roland Westrelin]

On Wed, 14 Dec 2022 09:55:27 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.

This pull request has now been integrated.

Changeset: 05a0a710
Author:    Roland Westrelin <roland at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/05a0a710313917fe7124ff43fe9c9af1d649bcac
Stats:     1690 lines in 25 files changed: 811 ins; 519 del; 360 mod

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

Reviewed-by: kvn, vlivanov

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

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