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

[Christian Hagedorn]

On Wed, 4 Jan 2023 17:04:19 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 incrementally with one additional commit since the last revision:
> 
>   Christian's review

Thanks for doing the updates, looks good!

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

Marked as reviewed by chagedorn (Reviewer).

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