8372634: C2: Materialize type information from instanceof checks by iwanowww · Pull Request #28517 · openjdk/jdk

iwanowww · 2025-11-27T00:53:54Z

Even though instanceof check (and reflective Class.isInstance call) narrows operand's type, sharpened type information is not explicitly materialized in the IR.

There's a SubTypeCheck node present, but it is not a substitute for a CheckCastPP node with a proper type.

The difference can be illustrated with the following simple cases:

   class A { void m() {} }
   class B extends A { void m() {} }

    void testInstanceOf(A obj) {
        if (obj instanceof B) {
            obj.m();
        }
    }

InstanceOf::testInstanceOf (12 bytes)
  @ 8 InstanceOf$A::m (0 bytes) failed to inline: virtual call

vs

    void testInstanceOfCast(A obj) {
        if (obj instanceof B) {
            B b = (B)obj;
            b.m();
        }
    }

InstanceOf::testInstanceOfCast (17 bytes)
  @ 13 InstanceOf$B::m (1 bytes) inline (hot)

Proposed fix annotates operands of subtype checks with proper type information which reflects the effects of subtype check. Not-yet-canonicalized IR shape poses some challenges, but I decided to match it early so information is available right away, rather than waiting for IGVN pass and delay inlining to post-parse phase.

FTR it is not a complete fix. It works for trivial cases, but for more complex conditions the IR shape becomes too complex during parsing (as illustrated by some test cases). I experimented with annotating subtype checks after initial parsing pass is over, but the crucial simplification step happens as part of split-if transformation which happens when no more inlining is possible. So, the only possible benefit (without forcing split-if optimization earlier) is virtual-to-direct call strength reduction. I plan to explore it separately.

Testing: hs-tier1 - hs-tier5

Progress

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

Issue

JDK-8372634: C2: Materialize type information from instanceof checks (Enhancement - P3)

Reviewers

Dean Long (@dean-long - Reviewer)
Quan Anh Mai (@merykitty - Committer) Review applies to 0a5e78c6
Roland Westrelin (@rwestrel - Reviewer) Review applies to 4e9f4624

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28517

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-27T00:54:48Z

👋 Welcome back vlivanov! 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 · 2025-11-27T00:55:09Z

@iwanowww This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8372634: C2: Materialize type information from instanceof checks

Reviewed-by: dlong, qamai, roland

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 113 new commits pushed to the master branch:

650de99: 8367158: C2: create better fill and copy benchmarks, taking alignment into account
325cdb7: 8373517: Revert the macos Tahoe specific change done in JDK-8359830
c46bed7: 8371502: serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java failing
... and 110 more: https://git.openjdk.org/jdk/compare/70e2bc876abe35b3d447f8004245bdbf2fead59f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-11-27T00:56:02Z

@iwanowww 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.

ExE-Boss · 2025-11-27T01:02:51Z

test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java

+    static boolean lateInlineInstanceOfCondPre(A o, boolean cond) {
+        return cond && (o instanceof B);
+    }
+
+    static boolean lateInlineInstanceOfCondPost(A o, boolean cond) {
+        return (o instanceof B) && cond;
+    }
+
+    static boolean lateInlineIsInstanceCondPre(A o, boolean cond) {
+        return cond && B.class.isInstance(o);
+    }
+    static boolean lateInlineIsInstanceCondPost(A o, boolean cond) {
+        return B.class.isInstance(o) && cond;
+    }


What about the non‑late version of these methods?

There are corresponding test cases (testInstanceOfCondPre et al) where conditions are embedded.

The idea of testInstanceOfCondLate and similar test cases is to check how inlining works when condition improves receiver type during incremental inlining phase.

What I meant was where the instanceof is in the called method, the testInstanceOfCondPre all have the instanceof checks as part of the if statement.

Something like:

static void testInstanceOfCondDefaultInlinePre(A a, boolean cond) { if (defaultInlineInstanceOfCondPre(a, cond)) { a.m(); } } static void testInstanceOfCondDefaultInlinePost(A a, boolean cond) { if (defaultInlineInstanceOfCondPost(a, cond)) { a.m(); } } static void testIsInstanceCondDefaultInlinePre(A a, boolean cond) { if (defaultInlineIsInstanceCondPre(a, cond)) { a.m(); } } static void testIsInstanceCondDefaultInlinePost(A a, boolean cond) { if (defaultInlineIsInstanceCondPost(a, cond)) { a.m(); } }

I suggest adding such a test because of real world code which use different internal implementation classes but expose their public API as only a single common supertype, like java.lang.constant.ClassDesc and its isPrimitive()/isArray()/isClassOrInterface() methods (which currently don’t do the instanceof check, but they probably should so that they can be reliably inlined).

The test is intended as a white-box test. It focuses on bytecode shapes which result in different IR representations and exercise different optimizations. From compiler perspective, there's no difference between if (defaultInlineInstanceOfCond(a)) { ... } and if (a instanceof B) {...} when inlining happens during parsing. Both test cases produce the very same IR after parsing is over.

It might be useful to have these tests in case the default inlining IR changes in the future.

It's intended as a unit test. It's better to catch inlining issue with targeted tests on inlining. From compiler perspective, there's no reason to cover other cases here. There are so many different scenarios how a subtype check can show up in IR. And different scenarios can theoretically fail due to different reasons.

mlbridge · 2025-11-27T01:21:24Z

Webrevs

rwestrel · 2025-11-27T13:25:27Z

I tried the patch and the test case out of curiosity but when I removed the change to Parse::sharpen_type_after_if(), the test still passed. I made the change below and it now fails without the change to Parse::sharpen_type_after_if() and passes with it.

diff --git a/test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java b/test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java
index cc292dc0900..bdb79966a2a 100644
--- a/test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java
+++ b/test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java
@@ -346,13 +346,14 @@ static boolean lateInlineIsInstanceCondPost(A o, boolean cond) {
     // Parse compilation log (-XX:+PrintCompilation -XX:+PrintInlining output).
     static boolean parseOutput(List<String> output) {
         boolean result = true;
-        Pattern compilation = Pattern.compile("^\\d+\\s+\\d+\\s+b\\s+");
+        Pattern compilation = Pattern.compile("^\\d+\\s+\\d+\\s+b\\s+.*");
         StringBuilder inlineTree = new StringBuilder();
         for (String line : output) {
             // Detect start of next compilation.
             if (compilation.matcher(line).matches()) {
                 // Parse output for previous compilation.
                 result &= validateInliningOutput(inlineTree.toString());
+                inlineTree.setLength(0);
             }
             inlineTree.append(line);
         }

merykitty · 2025-11-27T14:08:03Z

src/hotspot/share/opto/parse2.cpp

+    return true; // found
+  }
+
+  // Match an instanceof check.


We seem to require that the input of SubTypeCheck is not null. What do you think about allowing SubTypeCheck to accept null and return false?

Yes, it's a good idea and the right direction to move. While experimenting with a different enhancement, I noticed that a subtype check leaves a null check behind irrespective of whether the check goes away or not.

Unfortunately, there are some engineering considerations which complicates the change. SubTypeCheck is shared across all the places where subtype checks are performed, but checkcast and instanceof differ in the way null is handled. So, the proper way to fix it is to introduce a higher-level representation which implicitly handles nulls and then eventually lower it to SubTypeCheck and materialize null check if needed.

There are multiple ways without having to have yet another higher-level representation. The first one is that since SubTypeCheck does not accept null now, we can just choose one result for null. Choosing the instanceof approach may be a little more desirable, as it removes the need to perform this complicated match, and for checkcast we can manually insert a CheckCastPP anyway. Another solution is to have another input to SubTypeCheck which gives the result when the obj is null. On a whim, I kind of like this, as we can match both the checkcast and the instanceof pattern here, it also simplifies GraphKit::gen_checkcast, as we do not have to worry about "the cast that always succeeds will leave behind a null check".

Just a suggestion, though. This PR is fine as it is to me.

I agree it can be implemented without introducing new fancy IR nodes. The open question to me though is whether we can live without materializing null check until SubTypeCheck nodes are macro expanded. Otherwise, it'll turn into a gradual lowering through different representations.

BTW null checks which have to be merged back break the matching of the IR shape. Something to improve as a follow-up change. Added new test cases to track it.

iwanowww · 2025-12-01T19:42:20Z

Thanks, Roland! I slightly reworked the test to make it more robust.

iwanowww · 2025-12-02T20:44:23Z

Any reviews, please?

dean-long · 2025-12-03T02:19:46Z

src/hotspot/share/opto/parse2.cpp

+    (*obj) = extract_obj_from_klass_load(&gvn, val);
+    (*cast_type) = tcon->isa_klassptr()->as_instance_type();
+    return true; // found
+  }


The old code checked klass_is_exact() for this case, but the new code does not, so was it redundant, given we have a constant?

Yes, the check is redundant. Moreover, I tested the patch having the check turned into an assert.

dean-long

Looks reasonable, but I'm not an expert in this area.

iwanowww · 2025-12-09T00:42:13Z

Thanks for the reviews, Dean and Quan.

@rwestrel do you want to take a look?

rwestrel

Looks good to me.

eme64 · 2025-12-09T10:04:02Z

test/hotspot/jtreg/compiler/inlining/TestSubtypeCheckTypeInfo.java

+    // Parse compilation log (-XX:+PrintCompilation -XX:+PrintInlining output).
+    static int parseOutput(List<String> output) {
+        int successCount = 0;
+        Pattern compilation = Pattern.compile("^\\d+\\s+\\d+.*");
+        StringBuilder inlineTree = new StringBuilder();
+        for (String line : output) {
+            // Detect start of next compilation.
+            if (compilation.matcher(line).matches()) {
+                // Parse output for previous compilation.
+                successCount += (validateInliningOutput(inlineTree.toString()) ? 1 : 0);
+                inlineTree = new StringBuilder(); // reset
+            }
+            inlineTree.append(line);
+        }
+        // Process last compilation
+        successCount += (validateInliningOutput(inlineTree.toString()) ? 1 : 0);
+        return successCount;
+    }
+
+    // Sample:
+    //    213   42    b        compiler.inlining.TestSubtypeCheckTypeInfo::testIsInstanceCondLatePost (13 bytes)
+    static final Pattern TEST_CASE = Pattern.compile("^\\d+\\s+\\d+\\s+b\\s+" + TEST_CLASS_NAME + "::(\\w+) .*");


Drive by comment, no need to change things here now:
@iwanowww @chhagedorn Would it not be nice if we could do this kind of matching with the TestFramework? Instead of IR matching, just match the output of any compilation tracing / printing.

Indeed, that would be a much better way.

Also, -XX:+LogCompilation is a nice option since publishes all information in a structured way, but it would introduce a dependency on LogCompilation tool in the test library.

That's an interesting idea to think about more. But I it would be a separate concept next to @IR even though some code could probably be shared.

but it would introduce a dependency on LogCompilation tool in the test library.

The IR framework already uses LogCompilation to parse the IR dumps from. But I think that's quite an overhead - a lot of the information is not needed and makes the parsing logic more complicated.

I've been thinking about introducing a separate IR framework file for all relevant dumps, similar to dumping IGV dumps to a separate file. This will simplify things and only dumps relevant information. It will also have a positive impact on performance.

We could even go a step further and send the dumps over a port to the IR framework (similar to dumping the graph directly to the opened IGV over the network). Today, we already have sockets in place to send messages from the test VM to the driver VM. I guess we could extend that to also allow HotSpot to send dumps to the driver VM (it sounds feasible to do but would need to experiment with it).

Sending dumps from HotSpot to the IR framework has another benefit that HotSpot can provide all the information needed for the IR framework to figure out where this dump belongs to (i.e. no additional parsing of a file needed). This might also allow us to do IR matching in parallel to the test VM execution. This is much more efficient because today, the driver VM just waits until the test VM is finished and only then starts to do IR matching.

Anyway, that was just a digression about future ideas for the IR framework. What you have now to parse the output is the best we can do today.

iwanowww · 2025-12-10T21:19:46Z

Updated comments and improved the test (improved robustness and added new test cases with nulls). Please, re-review.

dean-long · 2025-12-12T01:55:19Z

Why the reversal on should_delay_inlining unification? Did it cause problems?

iwanowww · 2025-12-12T02:23:08Z

Why the reversal on should_delay_inlining unification? Did it cause problems?

Yes, there were some assertion failures observed during testing.

I thought that it is an equivalent change, but it turned out it's not. So, I reverted it. I plan to look into it separately.

iwanowww · 2025-12-12T21:10:51Z

Thanks for the reviews, Quan, Roland, and Dean.

/integrate

openjdk · 2025-12-12T21:12:11Z

Going to push as commit f2e56e4.
Since your change was applied there have been 126 commits pushed to the master branch:

4e9525e: 8373388: Reenable LTO for libsplashscreen
b6319f5: 8369595: HttpClient: HttpHeaders.firstValueAsLong failures should be converted to ProtocolException
6e2ab84: 8068378: [TEST_BUG]The java/awt/Modal/PrintDialogsTest/PrintDialogsTest.java instruction need to update
... and 123 more: https://git.openjdk.org/jdk/compare/70e2bc876abe35b3d447f8004245bdbf2fead59f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-12T21:12:22Z

@iwanowww Pushed as commit f2e56e4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

iwanowww added 2 commits November 26, 2025 16:15

C2: Materialize type information from instanceof checks

f054553

bugid

1cf6238

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 27, 2025

ExE-Boss reviewed Nov 27, 2025

View reviewed changes

iwanowww marked this pull request as ready for review November 27, 2025 01:17

openjdk bot added the rfr Pull request is ready for review label Nov 27, 2025

merykitty reviewed Nov 27, 2025

View reviewed changes

Test fix

0a5e78c

Unify Compile::should_delay_inlining

c58c63c

dean-long reviewed Dec 3, 2025

View reviewed changes

dean-long approved these changes Dec 3, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2025

merykitty approved these changes Dec 3, 2025

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Dec 3, 2025

Merge branch 'master' into c2.instanceof

58a7d52

Revert Compile::should_delay_inlining unification

4e9f462

rwestrel approved these changes Dec 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 9, 2025

eme64 reviewed Dec 9, 2025

View reviewed changes

iwanowww added 2 commits December 9, 2025 18:27

Improve comments

5aa5417

Improve the test

cff165a

openjdk bot removed the ready Pull request is ready to be integrated label Dec 10, 2025

dean-long approved these changes Dec 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 12, 2025

openjdk bot added the integrated Pull request has been integrated label Dec 12, 2025

openjdk bot closed this Dec 12, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 12, 2025

Conversation

iwanowww commented Nov 27, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 27, 2025

Uh oh!

openjdk bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

rwestrel commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww commented Dec 1, 2025

Uh oh!

iwanowww commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

iwanowww commented Dec 9, 2025

Uh oh!

rwestrel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww commented Dec 10, 2025

Uh oh!

dean-long commented Dec 12, 2025

Uh oh!

iwanowww commented Dec 12, 2025

Uh oh!

iwanowww commented Dec 12, 2025

Uh oh!

iwanowww commented Nov 27, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 27, 2025 •

edited

Loading

openjdk bot commented Nov 27, 2025 •

edited

Loading

iwanowww Dec 1, 2025 •

edited

Loading

mlbridge bot commented Nov 27, 2025 •

edited

Loading

iwanowww Dec 6, 2025 •

edited

Loading

iwanowww Dec 10, 2025 •

edited

Loading