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

[Christian Hagedorn]

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

The fix looks reasonable to me, too.

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

> 72:     return false;
> 73:   }
> 74:   if (!at_relevant_ctrl(n, blk1,blk2))

Suggestion:

  if (!at_relevant_ctrl(n, blk1, blk2))

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

> 86:   }
> 87: 
> 88:   if (process_cmp_loadklass(n, blk1, blk2)) {

I suggest to also rename this into something like `clone_cmp_loadklass_down` to follow the naming of `clone_cmp_down`.

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

> 267: }
> 268: 
> 269: void PhaseIdealLoop::process_load_klass_helper(const Node* n, Node* cmp, int i) {

Maybe you want to also rename this function into something like `clone_loadklass_nodes_at_cmp_index`.

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

Marked as reviewed by chagedorn (Reviewer).

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