8335256: [lworld] C2: Remove larval InlineTypeNode #1447

merykitty · 2025-04-27T13:17:10Z

Hi

The root issue is that a larval value object is fundamentally different from a non-larval one. The most important thing is that it has a unique identity and it expects any modification on 1 reference observable by all other equivalent references. As a result, we need a mechanism to track the identity of a larval object, which InlineTypeNode does not provide. My current proposal to fix this issue is to abandon larval InlineTypeNodes and use the oop like other objects.

It is probably beneficial to have another mechanism to make it easier optimizing larval inline type nodes, but I think it can be a follow-up RFE.

An example regarding the issue with tracking the identity of a larval InlineTypeNode:

Consider this pseudobytecode sequence:

new MyValue;
dup;
loop;
invokespecial MyValue::<init>;
areturn;

There are 2 equivalent references in the stack at the loop entry. When Parse::merge encounters them, it cannot know that these are the same because the back-edge has not been processed yet. As a result, it creates 2 separate Phis for these references. Then, invokespecial will only make the top of the stack a non-larval object, not the next one, which is the one returned to the caller. As a result, we fail with assert(!value->is_InlineType() || !value->as_InlineType()->is_larval(), "returning a larval"). Worse, if the method is osr-compiled at the loop head, we have 2 separate references fed into the compiled function and there is no way we may know that they are of the same object.

Please take a look and leave your reviews, thanks a lot.

Progress

Change must not contain extraneous whitespace

Issues

JDK-8335256: [lworld] C2: Remove larval InlineTypeNode (Bug - P3)
JDK-8325627: [lworld] C2 compilation bailout for TestNullableInlineTypes::test85 (Bug - P4)
JDK-8354283: [lworld] TestAllocationMergeAndFolding fails since jdk-25+16, since an allocation is detected (Bug - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1447

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1447.diff

Using Webrev

Link to Webrev Comment

merykitty · 2025-04-27T13:17:52Z

/issue 8325627
/issue 8354283

bridgekeeper · 2025-04-27T13:18:06Z

👋 Welcome back qamai! A progress list of the required criteria for merging this PR into lworld 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-04-27T13:18:37Z

@merykitty 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:

8335256: [lworld] C2: Remove larval InlineTypeNode
8325627: [lworld] C2 compilation bailout for TestNullableInlineTypes::test85
8354283: [lworld] TestAllocationMergeAndFolding fails since jdk-25+16, since an allocation is detected

Reviewed-by: thartmann

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 2 new commits pushed to the lworld branch:

cc76937: 8348959: [lworld] compiler/c2/TestMergeStores.java fails IR verification
c65b458: 8356599: [lworld] C2 incorrectly folds array object klass load for atomic arrays

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-04-27T13:19:16Z

@merykitty
Adding additional issue to issue list: 8325627: [lworld] C2 compilation bailout for TestNullableInlineTypes::test85.

openjdk · 2025-04-27T13:19:57Z

@merykitty
Adding additional issue to issue list: 8354283: [lworld] TestAllocationMergeAndFolding fails since jdk-25+16, since an allocation is detected.

mlbridge · 2025-04-27T13:23:02Z

Webrevs

TobiHartmann · 2025-04-29T08:36:24Z

Great work @merykitty! Thanks a lot for the investigation. When working on the new value object construction model, I was concerned that we might end up with adding identity back to the larvals but I was still naively hoping that we could get around that. Looks like we can't. Maybe you could change the title to something more descriptive for future reference / search.

I run this through testing and there are a few issues:

applications/ctw/modules/jdk_internal_le.java
-ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/workspace/open/src/hotspot/share/opto/macro.cpp:2970), pid=1140043, tid=1140412
#  assert(!n->as_AbstractLock()->is_eliminated()) failed: sanity
#
# JRE version: Java(TM) SE Runtime Environment (25.0) (fastdebug build 25-lworld5ea-LTS-2025-04-28-0856180.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 25-lworld5ea-LTS-2025-04-28-0856180.tobias.hartmann.valhalla2, mixed mode, sharing, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x15359a4]  PhaseMacroExpand::eliminate_macro_nodes()+0x714

Current CompileTask:
C2:26441 3356   !b        jdk.internal.org.jline.utils.NonBlockingInputStreamImpl::run (312 bytes)

Stack: [0x00007f27dbbfd000,0x00007f27dbcfd000],  sp=0x00007f27dbcf8250,  free space=1004k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x15359a4]  PhaseMacroExpand::eliminate_macro_nodes()+0x714  (macro.cpp:2970)
V  [libjvm.so+0xb26617]  Compile::Optimize()+0x1867  (compile.cpp:2903)
V  [libjvm.so+0xb28ceb]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1feb  (compile.cpp:874)
V  [libjvm.so+0x94e98d]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x46d  (c2compiler.cpp:142)
V  [libjvm.so+0xb379f2]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xb22  (compileBroker.cpp:2307)
V  [libjvm.so+0xb389f8]  CompileBroker::compiler_thread_loop()+0x588  (compileBroker.cpp:1951)
V  [libjvm.so+0x109191f]  JavaThread::thread_main_inner()+0x12f  (javaThread.cpp:773)
V  [libjvm.so+0x1abf606]  Thread::call_run()+0xb6  (thread.cpp:231)
V  [libjvm.so+0x17466d8]  thread_native_entry(Thread*)+0x128  (os_linux.cpp:877)

compiler/valhalla/inlinetypes/TestValueClasses.java
-DWarmup=0 -DVerifyIR=false 
OR
-Xcomp -XX:-TieredCompilation -DIgnoreCompilerControls=true

Caused by: java.lang.RuntimeException: assertEquals expected: compiler.valhalla.inlinetypes.TestValueClasses$SmallNullable1@b0186fd4 but was: compiler.valhalla.inlinetypes.TestValueClasses$SmallNullable1@6279ef0a
	at jdk.test.lib.Asserts.fail(Asserts.java:715)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:208)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:195)
	at jdk.test.lib.Asserts.assertEQ(Asserts.java:172)
	at compiler.valhalla.inlinetypes.TestValueClasses.test9_verifier(TestValueClasses.java:434)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	... 7 more

compiler/valhalla/inlinetypes/TestValueConstruction.java
-XX:+UnlockDiagnosticVMOptions -XX:+AbortVMOnCompilationFailure

This is expected but to be able to run this test with `-XX:+AbortVMOnCompilationFailure`, which is valuable, maybe we should put your new test in another jtreg test and hardcode `-XX:-AbortVMOnCompilationFailure` there?

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (workspace/open/src/hotspot/share/compiler/compileBroker.cpp:2114), pid=2843210, tid=2843229
#  fatal error: Not compilable at tier 4: OSR starts with non-empty stack
#
# JRE version: Java(TM) SE Runtime Environment (25.0) (fastdebug build 25-lworld5ea-LTS-2025-04-28-0856180.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 25-lworld5ea-LTS-2025-04-28-0856180.tobias.hartmann.valhalla2, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0xb30beb]  CompileBroker::handle_compile_error(CompilerThread*, CompileTask*, ciEnv*, int, char const*)+0x6b

Current CompileTask:
C2:6345 1087 %  b  4       compiler.valhalla.inlinetypes.TestValueConstruction$Code_0::multipleOccurrencesInJVMSReturnStack @ 5 (22 bytes)

Stack: [0x00007fc85457f000,0x00007fc85467f000],  sp=0x00007fc85467d450,  free space=1017k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0xb30beb]  CompileBroker::handle_compile_error(CompilerThread*, CompileTask*, ciEnv*, int, char const*)+0x6b  (compileBroker.cpp:2114)
V  [libjvm.so+0xb3734c]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x47c  (compileBroker.cpp:2340)
V  [libjvm.so+0xb389f8]  CompileBroker::compiler_thread_loop()+0x588  (compileBroker.cpp:1951)
V  [libjvm.so+0x109191f]  JavaThread::thread_main_inner()+0x12f  (javaThread.cpp:773)
V  [libjvm.so+0x1abf606]  Thread::call_run()+0xb6  (thread.cpp:231)
V  [libjvm.so+0x17466d8]  thread_native_entry(Thread*)+0x128  (os_linux.cpp:877)

compiler/valhalla/inlinetypes/TestValueConstruction.java

Various flags:
- No flags
- "-XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressArrayCopyMacroNode -XX:+StressCallingConvention -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP -XX:+StressIGVN -XX:+StressReflectiveCode -XX:+StressMethodHandleLinkerInlining -XX:+StressCompiledExceptionHandlers -XX:MaxNodeLimit=100000"

java.lang.RuntimeException: assertEquals expected: x: 0 but was: x: 50000
	at jdk.test.lib.Asserts.fail(Asserts.java:715)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:208)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:195)
	at jdk.test.lib.Asserts.assertEQ(Asserts.java:172)
	at compiler.valhalla.inlinetypes.TestValueConstruction.check(TestValueConstruction.java:1851)
	at compiler.valhalla.inlinetypes.TestValueConstruction.run(TestValueConstruction.java:1846)
	at compiler.valhalla.inlinetypes.TestValueConstruction.main(TestValueConstruction.java:1773)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:1447)

merykitty · 2025-04-29T20:00:11Z

@TobiHartmann Thanks a lot for the comprehensive testing, it is really helpful.

applications/ctw/modules/jdk_internal_le.java
-ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation

This one fails because after removal of some allocations, not only other allocations become eligible for elimination, locks can also be eliminated. As a result, we need to do the whole macro elimination again. I noticed that _has_lock is unused, which makes it trivial to merge the 2 loops, simplify the implementation there.

compiler/valhalla/inlinetypes/TestValueClasses.java
-DWarmup=0 -DVerifyIR=false
OR
-Xcomp -XX:-TieredCompilation -DIgnoreCompilerControls=true

This one is an oversight by me. When exiting a constructor, the larval oop is aggressively replaced by the non-larval one in all the frames. This relies on the assumption that all blocks in which the oop is larval has been processed. This is incorrect when we have uncommon traps, which can be processed at any time. As a result, we can only replace the larval oop with the non-larval one in the current frame.

compiler/valhalla/inlinetypes/TestValueConstruction.java
-XX:+UnlockDiagnosticVMOptions -XX:+AbortVMOnCompilationFailure

I modified the test to push everything into the local slots, 2 distinct slots are present into the loop, keep the original intention of the test.

compiler/valhalla/inlinetypes/TestValueConstruction.java
Various flags:

No flags

"-XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation

This is interesting. This is an oversight from #1424. The issue is that it is hard to decide that 2 nodes are definitely different or definitely the same, especially when the graph may be non-canonical during IGVN. As a result, I resort to a more conservative approach there:

Give up finding a matching constructor for a local allocation.
When deciding if a call can modify a field of a local allocation, don't take into account the receiver and return true for any constructor of a subclass regardless of whether the receiver is actually the object we are loading from.

TobiHartmann · 2025-05-05T12:02:46Z

Thanks for quickly fixing these issues. I ran another round of testing and there's only one issue left:

compiler/arraycopy/TestEliminatedArrayCopyPhi.java
-ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/workspace/open/src/hotspot/share/opto/phaseX.cpp:1821), pid=1040062, tid=1040080
#  Error: assert(_worklist.size() == 0) failed
#
# JRE version: Java(TM) SE Runtime Environment (25.0) (fastdebug build 25-lworld5ea-LTS-2025-04-30-0702055.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 25-lworld5ea-LTS-2025-04-30-0702055.tobias.hartmann.valhalla2, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x17de609]  PhaseCCP::PhaseCCP(PhaseIterGVN*)+0x169

Current CompileTask:
C2:455  160    b  4       compiler.arraycopy.TestEliminatedArrayCopyPhi::test (27 bytes)

Stack: [0x00007fadf8385000,0x00007fadf8485000],  sp=0x00007fadf8480270,  free space=1004k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x17de609]  PhaseCCP::PhaseCCP(PhaseIterGVN*)+0x169  (phaseX.cpp:1821)
V  [libjvm.so+0xb2573a]  Compile::Optimize()+0xa0a  (compile.cpp:2950)
V  [libjvm.so+0xb28b7b]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1feb  (compile.cpp:874)
V  [libjvm.so+0x94e98d]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x46d  (c2compiler.cpp:142)
V  [libjvm.so+0xb37882]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xb22  (compileBroker.cpp:2307)
V  [libjvm.so+0xb38888]  CompileBroker::compiler_thread_loop()+0x588  (compileBroker.cpp:1951)
V  [libjvm.so+0x109182f]  JavaThread::thread_main_inner()+0x12f  (javaThread.cpp:773)
V  [libjvm.so+0x1ac19d6]  Thread::call_run()+0xb6  (thread.cpp:231)
V  [libjvm.so+0x1748888]  thread_native_entry(Thread*)+0x128  (os_linux.cpp:877)

merykitty · 2025-05-06T00:08:41Z

@TobiHartmann The failure was due to the fact that PhaseMacroExpand::eliminate_allocate_node can push nodes on the igvn worklist even if it does not successfully remove the allocation. As a result, I moved the progress check to after igvn.optimize() so that igvn worklist is cleared.

TobiHartmann

Thanks a lot for this great work and your patience, @merykitty. All tests passed and I had a detailed look at the changes. Looks all good to me, I just left a few questions / comments / suggestions.

TobiHartmann · 2025-05-09T07:26:48Z

src/hotspot/share/opto/compile.cpp

@@ -2843,16 +2843,39 @@ void Compile::Optimize() {

  if (failing())  return;

+  {
+    // Eliminate some macro nodes before EA to reduce analysis pressure


Is this something we should consider doing in mainline?

I'm not sure what effect it will have on mainline. For this patch, without this, TestNullableInlineTypes::test85 tries to enter EA with 10k nodes, which it bails out. Trying to eliminate all trivially removable allocations reduces the number to 2k, allows EA to complete successfully. I think it may be more significant on Valhalla because in general we seem to emit a lot more nodes with value objects.

Please let me know what you think about this, perhaps we need to measure the effects on mainline first.

Makes sense. I'd say we put it into Valhalla only for now. We can still consider upstreaming it later.

TobiHartmann · 2025-05-09T07:27:47Z

src/hotspot/share/opto/compile.cpp

        igvn.set_delay_transform(false);
-        igvn.optimize();
-        if (failing()) return;
-
        print_method(PHASE_ITER_GVN_AFTER_ELIMINATION, 2);


Why did you remove the igvn.optimize(); here?

Ah, you added it to eliminate_macro_nodes.

Yes, I think it is cleaner that way instead of trailing each mex.eliminate_macro_nodes with an igvn.optimize()

src/hotspot/share/opto/doCall.cpp

src/hotspot/share/opto/graphKit.cpp

src/hotspot/share/opto/parse3.cpp

src/hotspot/share/opto/parse1.cpp

merykitty · 2025-05-09T18:10:45Z

@TobiHartmann Thanks a lot for your reviews, I think I have addressed all of them.

TobiHartmann

Thanks for the nice comments and updates, Quan-Anh. The changes look good to me now!

merykitty · 2025-05-12T13:36:52Z

Thanks very much for the reviews, I have created JDK-8356779 and JDK-8356780.
/integrate

openjdk · 2025-05-12T13:37:36Z

@merykitty
Your change (at version 4b16f6d) is now ready to be sponsored by a Committer.

TobiHartmann · 2025-05-12T14:32:28Z

/sponsor

openjdk · 2025-05-12T14:32:47Z

Going to push as commit 6a6cf4f.
Since your change was applied there have been 2 commits pushed to the lworld branch:

cc76937: 8348959: [lworld] compiler/c2/TestMergeStores.java fails IR verification
c65b458: 8356599: [lworld] C2 incorrectly folds array object klass load for atomic arrays

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-12T14:32:56Z

@TobiHartmann @merykitty Pushed as commit 6a6cf4f.

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

remove larval InlineTypeNode

ae3ed36

openjdk bot added ready rfr labels Apr 27, 2025

Merge branch 'lworld' into larvaloop

d9e233e

fix test failures

d91fcf9

fast path for non intrinsics

3be7b97

merykitty changed the title ~~8335256: [lworld] compiler/valhalla/inlinetypes/TestValueConstruction.java fails intermittently~~ 8335256: [lworld] C2: Remove larval InlineTypeNode Apr 30, 2025

merykitty added 2 commits May 6, 2025 06:27

Merge branch 'lworld' into larvaloop

9cedae4

move progress check

bdc01f4

TobiHartmann approved these changes May 9, 2025

View reviewed changes

Tobias' comments

4b16f6d

TobiHartmann approved these changes May 12, 2025

View reviewed changes

openjdk bot added the sponsor label May 12, 2025

openjdk bot added the integrated label May 12, 2025

openjdk bot closed this May 12, 2025

openjdk bot removed ready rfr sponsor labels May 12, 2025

8335256: [lworld] C2: Remove larval InlineTypeNode #1447

8335256: [lworld] C2: Remove larval InlineTypeNode #1447

Uh oh!

Conversation

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

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

merykitty commented Apr 27, 2025

Uh oh!

bridgekeeper bot commented Apr 27, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Apr 27, 2025

Uh oh!

openjdk bot commented Apr 27, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

TobiHartmann commented Apr 29, 2025

Uh oh!

merykitty commented Apr 29, 2025

Uh oh!

TobiHartmann commented May 5, 2025

Uh oh!

merykitty commented May 6, 2025

Uh oh!

TobiHartmann left a comment • 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!

TobiHartmann May 9, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty May 9, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty May 9, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann May 12, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann May 9, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann May 9, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

merykitty commented May 9, 2025

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented May 12, 2025

Uh oh!

openjdk bot commented May 12, 2025

Uh oh!

TobiHartmann commented May 12, 2025

Uh oh!

openjdk bot commented May 12, 2025

Uh oh!

merykitty commented Apr 27, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 27, 2025 •

edited

Loading

mlbridge bot commented Apr 27, 2025 •

edited

Loading

TobiHartmann left a comment •

edited

Loading