RFR: 8298848: C2: clone all of (CmpP (LoadKlass (AddP down at split if [v2]

[Tobias Hartmann]

On Wed, 21 Dec 2022 15:18:23 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> As suggested by Vladimir in:
>> https://github.com/openjdk/jdk/pull/11666
>> 
>> Thus extract one for the fixes as a separate PR. The bug as described
>> in the above PR is:
>> 
>> 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 `CheckCastPP`s 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.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - fix test on x86 32 bits
>  - Merge branch 'master' into JDK-8298848
>  - test & fix

Looks reasonable to me.

src/hotspot/share/opto/split_if.cpp line 241:

> 239:             --j;
> 240: 
> 241:           }

Suggestion:

            --j;
          }

src/hotspot/share/opto/split_if.cpp line 309:

> 307:     }
> 308: 
> 309:   }

Suggestion:

    }
  }

src/hotspot/share/opto/split_if.cpp line 423:

> 421:         _igvn.replace_input_of(use, pos, x);
> 422:       }
> 423:       _igvn.remove_dead_node(n );

Suggestion:

      _igvn.remove_dead_node(n);

test/hotspot/jtreg/compiler/types/TestCheckCastPPBecomesTOP.java line 2:

> 1: /*
> 2:  * Copyright (c) 2022, Red Hat, Inc. All rights reserved.

Suggestion:

 * Copyright (c) 2023, Red Hat, Inc. All rights reserved.

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

Marked as reviewed by thartmann (Reviewer).

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