Integrated: 8298848: C2: clone all of (CmpP (LoadKlass (AddP down at split if

[Roland Westrelin]

On Thu, 15 Dec 2022 10:53:37 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.

This pull request has now been integrated.

Changeset: 87238470
Author:    Roland Westrelin <roland at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/872384707e89d03ede655aad16f360dc94f10152
Stats:     556 lines in 3 files changed: 440 ins; 104 del; 12 mod

8298848: C2: clone all of (CmpP (LoadKlass (AddP down at split if

Reviewed-by: kvn, thartmann, chagedorn

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

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