8381546: C2: Improve static checks on SubTypeCheck

[hgqxjj]

Description:

There are scenarios where SubTypeCheckNode was not eliminated when receiver implements an interface not related to the class being checked against.

Solution:

This PR enhances maybe_java_subtype_of_helper_for_instance by using CHA to improve the target type and verify it against the receiver's interface constraints. If the improved leaf type does not implement the required interfaces, the relationship is proven impossible and the node is folded.

Test:

GHA

Please review this change. Thanks!

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8381546: C2: Improve static checks on SubTypeCheck (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30690/head:pull/30690
$ git checkout pull/30690

Update a local copy of the PR:
$ git checkout pull/30690
$ git pull https://git.openjdk.org/jdk.git pull/30690/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30690

View PR using the GUI difftool:
$ git pr show -t 30690

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30690.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back ghan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@hgqxjj The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (908bb8ad)
• 03: Full - Incremental (34f42664)
• 02: Full - Incremental (f9744c48)
• 01: Full - Incremental (e516582c)
• 00: Full (304c7d8c)

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[dafedafe]

Thanks a lot for looking into this @hgqxjj. The fix looks reasonable to me: I just have a couple of questions.

[dafedafe]

Wow, this is quite a test! I suppose new C() in the array is there to "force" C to be loaded, right?

[hgqxjj]

Hi @dafedafe, yes , new C() is there to make sure the compiler sees that B has a loaded final subclass.

[dafedafe]

It would be nice to have a test that checks when a later class loading breaks the leaf assumption, but maybe we already have a more generic one...

[hgqxjj]

That's a good point. However, this patch only reuses the existing assert_leaf_type() dependency mechanism, so I did not add a separate test for later class loading. That should already be covered by the generic dependency tests.

[merykitty]

Can you improve the static checks of SubTypeCheck by going through the same analysis as CheckCastPP instead? I think consolidating the 2 analysis is preferable.

[iwanowww]

I agree with @merykitty that it would be better to unify SubTypeCheck analysis with receiver type computation. Or, at least, verify that both agree.

[iwanowww]

It's the wrong place to improve the type. The culprit is that while GraphKit::gen_checkcast() tries to improve the type, GraphKit::gen_instanceof() does not which is a missed optimization opportunity. IMO it's better to shape it as a separate RFE.

[hgqxjj]

Can you improve the static checks of SubTypeCheck by going through the same analysis as CheckCastPP instead? I think consolidating the 2 analysis is preferable.

I agree with @merykitty that it would be better to unify SubTypeCheck analysis with receiver type computation. Or, at least, verify that both agree.

Hi @merykitty and @iwanowww , Sorry for late response.

I have recently explored the possibility of unifying the SubTypeCheck analysis with receiver type computation (using filter() or higher_equal and so on). However, my findings suggest that a complete unification is difficult, Specifically, I’d like to highlight two scenarios where type computation based logic fails to align with the requirements of static_subtype_check

1.Risk of Missing SSC_always_true Cases

When using type lattice operations to determine SSC_always_true, such as:

const Type* tboth = subk->filter(superk);
if (Type::equals(tboth, subk)) {
  return SSC_always_true;
} 

or

if (subk->higher_equal(superk)){
    return SSC_always_true;
 }

This logic frequently triggers assertions in SubTypeCheckNode::verify_helper. The detailed error log as below:

35  ConP  === 0  [[ 36 42 42 ]]  #instklassptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):Constant+0  Klass:instklassptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):Constant+0

 10  Parm  === 3  [[ 4 21 24 21 25 25 25 34 34 34 36 38 38 ]] Parm0: instptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):NotNull+0,iid=bot  Oop:instptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):NotNull+0,iid=bot !jvms: BufferedInputStream::read @ bci:-1 (line 379)

 36  SubTypeCheck  === _ 10 35  [[ ]]  profiled at: java.io.BufferedInputStream::read:1 !jvms: BufferedInputStream::read @ bci:1 (line 379)

The value of cmp_t in SubTypeCheckNode::verify_helper is TypeInt::CC_EQ.

This implies that type lattice operations failed to capture some cases that should be SSC_always_true.

However, this case is benign for CheckCastPP, as it just miss a chance of narrowing type.

2. The Risk of Wrongly Deleting Branches

Using filter() == Type::TOP to return SSC_always_false is risky . Consider the following:

const Type* tboth = sub_t->filter(super_t);
if (tboth == Type::TOP) {
  return SSC_always_false;
}

filter() operates based on the currently loaded classes.

filter() will returns Type::TOP when sub_t (e.g., Object with {I1, I2}) and super_t (e.g., Object with {I3, I4, I5}) share no common subtypes among currently loaded classes. Consequently, the corresponding branch would be statically deleted.

However, the JVM may later load a new class that implements all interfaces {I1, I2, I3, I4, I5}. To my knowledge, there is no existing Dependency mechanism to detect such a class loading event and trigger the necessary deoptimization to correct the compiled code . While I am aware of existing dependency types like assert_leaf_type, they are not suitable for this specific case.

However, this case is benign for CheckCastPP.

I would appreciate your thoughts on whether we should accept this divergence as a design necessity or if you see a viable path for a safe, dependency-backed unification.

Thanks!

[iwanowww]

I can only guess how the patch looked like, but there are 2 peculiarities when it comes to SubTypeCheck:

• reflective case (when super is not a constant);
• in non-reflective case, superclass is a constant

In the latter case, it's not correct to use superk as is. You have to cast "exactness" bit away first.

I made an experiment [1] and spotted only a single divergence with Compile::static_subtype_check due to leaf assertion on superk.

[1] https://github.com/openjdk/jdk/compare/master...iwanowww:jdk:sub_type?expand=1

[openjdk]

⚠️ @hgqxjj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[hgqxjj]

The patch has been updated with the following changes:

1. Improve klass_ptr_type in GraphKit::gen_instanceof() similarly to GraphKit::gen_checkcast()
2. Refactor static_subtype_check to make better use of type analysis.
3. Fix the “Meet not symmetric” issue for TypeAryKlassPtr types, which was exposed by the refactoring in item 2.

@iwanowww Could you please take a look?

[merykitty]

1.Risk of Missing SSC_always_true Cases

When using type lattice operations to determine SSC_always_true, such as:

const Type* tboth = subk->filter(superk);
if (Type::equals(tboth, subk)) {
  return SSC_always_true;
} 

or

if (subk->higher_equal(superk)){
    return SSC_always_true;
}

This logic frequently triggers assertions in SubTypeCheckNode::verify_helper. The detailed error log as below:

35  ConP  === 0  [[ 36 42 42 ]]  #instklassptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):Constant+0  Klass:instklassptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):Constant+0

10  Parm  === 3  [[ 4 21 24 21 25 25 25 34 34 34 36 38 38 ]] Parm0: instptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):NotNull+0,iid=bot  Oop:instptr:java/io/BufferedInputStream (java/lang/AutoCloseable,java/io/Closeable):NotNull+0,iid=bot !jvms: BufferedInputStream::read @ bci:-1 (line 379)

36  SubTypeCheck  === _ 10 35  [[ ]]  profiled at: java.io.BufferedInputStream::read:1 !jvms: BufferedInputStream::read @ bci:1 (line 379)

The value of cmp_t in SubTypeCheckNode::verify_helper is TypeInt::CC_EQ.

This implies that type lattice operations failed to capture some cases that should be SSC_always_true.

However, this case is benign for CheckCastPP, as it just miss a chance of narrowing type.

Looking at the dump, I'm fairly certain you are checking the incorrect thing. The correct thing to check is to verify that the type of an oop that would satisfy the SubTypeCheck would be a subtype of the type of the input of the SubTypeCheck. The former is only known when the klass input is a constant, in which case it is anything that is a subtype of that klass pointer constant.

if (superk->singleton() && subt->higher_equal(superk->as_instance_type()->cast_to_exactness(false)))

2. The Risk of Wrongly Deleting Branches

Using filter() == Type::TOP to return SSC_always_false is risky . Consider the following:

const Type* tboth = sub_t->filter(super_t);
if (tboth == Type::TOP) {
  return SSC_always_false;
}

filter() operates based on the currently loaded classes.

filter() will returns Type::TOP when sub_t (e.g., Object with {I1, I2}) and super_t (e.g., Object with {I3, I4, I5}) share no common subtypes among currently loaded classes. Consequently, the corresponding branch would be statically deleted.

However, the JVM may later load a new class that implements all interfaces {I1, I2, I3, I4, I5}. To my knowledge, there is no existing Dependency mechanism to detect such a class loading event and trigger the necessary deoptimization to correct the compiled code . While I am aware of existing dependency types like assert_leaf_type, they are not suitable for this specific case.

However, this case is benign for CheckCastPP.

No, it is not benign for CheckCastPP, if the CheckCastPP finds that its type is top, we either kill the path regardless when KillPathsReachableByDeadTypeNode is true, or fail the compilation somewhere down the line otherwise. The issue is that Type::filter is not allowed to return a result that may be incorrect if classes are loaded/unloaded in the future.

[iwanowww]

@hgqxjj Thanks for looking into it.

1. Improve klass_ptr_type in GraphKit::gen_instanceof() similarly to GraphKit::gen_checkcast()

I suggest to upstream it separately.

3. Fix the “Meet not symmetric” issue for TypeAryKlassPtr types, which was exposed by the refactoring in item 2.

Are you talking about JDK-8383823? It's better to fix it separately as well.

[iwanowww]

I have been playing with the prototype [1] in background and got some more insights.

1. Oop-vs-klass and klass-vs-klass comparisons are different. In the former case, it's possible to assert that the subclass has to be instantiatable (thus enabling more optimization opportunities).

2. It may be counter-productive to optimistically narrow types based on CHA: if it doesn't improve the subtype check, then it ends up with the same compiled code and more nmethod dependencies, so the code becomes less resilient to future class loading. Of course, it can benefit subsequent code, but then those cases can be optimized separately irrespective of SubTypeCheck presence. Not sure how problematic it is though. In the worst case, it causes recompilation and TypeKlassPtr::try_improve() already suffers from it.

[1] master...iwanowww:jdk:sub_type

[iwanowww]

Why does superk_e->klass_is_exact() part make a difference?

[hgqxjj]

My understanding is that superk_e->klass_is_exact() can become true while superk_is_exact is still false when a non-final super klass currently has no subclasses. In that case, superk_e->klass_is_exact() together with subk_e_higher proves that the subtype check is always true.

[iwanowww]

Right, I was thinking about whether superk_e->klass_is_exact() implies superk_is_exact == true, but it's evidently not the case. (Forgot that superk_is_exact is captured on the original superk before its "constness" is casted away.)

[hgqxjj]

1. Improve klass_ptr_type in GraphKit::gen_instanceof() similarly to GraphKit::gen_checkcast()

I suggest to upstream it separately.

3. Fix the “Meet not symmetric” issue for TypeAryKlassPtr types, which was exposed by the refactoring in item 2.

Are you talking about JDK-8383823? It's better to fix it separately as well.

OK, I will split them out and upstream them separately.

I have been playing with the prototype [1] in background and got some more insights.

1. Oop-vs-klass and klass-vs-klass comparisons are different. In the former case, it's possible to assert that the subclass has to be instantiatable (thus enabling more optimization opportunities).
2. It may be counter-productive to optimistically narrow types based on CHA: if it doesn't improve the subtype check, then it ends up with the same compiled code and more nmethod dependencies, so the code becomes less resilient to future class loading. Of course, it can benefit subsequent code, but then those cases can be optimized separately irrespective of SubTypeCheck presence. Not sure how problematic it is though. In the worst case, it causes recompilation and TypeKlassPtr::try_improve() already suffers from it.

[1] master...iwanowww:jdk:sub_type

Thank you for the explanation. I will continue refining these changes

[hgqxjj]

Looking at the dump, I'm fairly certain you are checking the incorrect thing. The correct thing to check is to verify that the type of an oop that would satisfy the SubTypeCheck would be a subtype of the type of the input of the SubTypeCheck. The former is only known when the klass input is a constant, in which case it is anything that is a subtype of that klass pointer constant.

No, it is not benign for CheckCastPP, if the CheckCastPP finds that its type is top, we either kill the path regardless when KillPathsReachableByDeadTypeNode is true, or fail the compilation somewhere down the line otherwise. The issue is that Type::filter is not allowed to return a result that may be incorrect if classes are loaded/unloaded in the future.

@merykitty Yes, that makes sense. I misunderstood some details here, thank you for the clarification. I will refine the changes with these points in mind.