-
Notifications
You must be signed in to change notification settings - Fork 141
8370880: [lworld] Split ciObjArrayKlass into ciRefArrayKlass and ciFlatArrayKlass #1739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
@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: 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
There are still a lot of refactoring to be done regarding moving elements from |
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this, Quan-Anh! Testing looks good except for this:
compiler/valhalla/inlinetypes/TestArrayNullMarkers.java#AVF-nAVF
-XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation -XX:-DoEscapeAnalysis -XX:+AlwaysIncrementalInline
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/workspace/open/src/hotspot/share/ci/ciMetadata.hpp:105), pid=418142, tid=418159
# assert(is_flat_array_klass()) failed: bad cast
#
# JRE version: Java(TM) SE Runtime Environment (26.0) (fastdebug build 26-jep401ea2-2025-11-17-0811237.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 26-jep401ea2-2025-11-17-0811237.tobias.hartmann.valhalla2, mixed mode, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V [libjvm.so+0x1ca876a] TypeAryPtr::flat_layout_helper() const+0x7a
Current CompileTask:
C2:2887 380 % !b compiler.valhalla.inlinetypes.TestArrayNullMarkers::main @ 698 (2967 bytes)
Stack: [0x00007f5639cfe000,0x00007f5639dfe000], sp=0x00007f5639df8240, free space=1000k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x1ca876a] TypeAryPtr::flat_layout_helper() const+0x7a (ciMetadata.hpp:105)
V [libjvm.so+0x102a8a9] GraphKit::get_layout_helper(Node*, int&)+0x299 (graphKit.cpp:4100)
V [libjvm.so+0x102cd43] GraphKit::new_array(Node*, Node*, int, Node**, bool, Node*)+0x43 (graphKit.cpp:4345)
V [libjvm.so+0x158b712] LibraryCallKit::inline_newArray(bool, bool)+0x242 (library_call.cpp:4904)
V [libjvm.so+0x15b2c6a] LibraryIntrinsic::generate(JVMState*)+0x22a (library_call.cpp:130)
V [libjvm.so+0xdbc50f] Parse::do_call()+0xebf (doCall.cpp:771)
V [libjvm.so+0x18fee88] Parse::do_one_bytecode()+0x4c8 (parse2.cpp:3526)
V [libjvm.so+0x18e6007] Parse::do_one_block()+0x357 (parse1.cpp:1703)
V [libjvm.so+0x18e7300] Parse::do_all_blocks()+0x130 (parse1.cpp:760)
V [libjvm.so+0x18eae81] Parse::Parse(JVMState*, ciMethod*, float)+0xea1 (parse1.cpp:664)
V [libjvm.so+0x9e1975] ParseGenerator::generate(JVMState*)+0x135 (callGenerator.cpp:99)
V [libjvm.so+0x9e6ba3] CallGenerator::do_late_inline_helper()+0xa03 (callGenerator.cpp:751)
V [libjvm.so+0xbdc968] Compile::inline_incrementally_one()+0x1b8 (compile.cpp:2614)
V [libjvm.so+0xbde0ab] Compile::inline_incrementally(PhaseIterGVN&)+0x38b (compile.cpp:2707)
V [libjvm.so+0xbe0c9e] Compile::Optimize()+0x47e (compile.cpp:2843)
V [libjvm.so+0xbe45c5] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1da5 (compile.cpp:879)
V [libjvm.so+0x9de490] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x4e0 (c2compiler.cpp:149)
V [libjvm.so+0xbf3e20] CompileBroker::invoke_compiler_on_method(CompileTask*)+0x780 (compileBroker.cpp:2345)
V [libjvm.so+0xbf5680] CompileBroker::compiler_thread_loop()+0x530 (compileBroker.cpp:1989)
V [libjvm.so+0x118d15b] JavaThread::thread_main_inner()+0x13b (javaThread.cpp:772)
V [libjvm.so+0x1c68ee6] Thread::call_run()+0xb6 (thread.cpp:243)
V [libjvm.so+0x1893bc8] thread_native_entry(Thread*)+0x128 (os_linux.cpp:883)
|
Thanks for your testing. Unfortunately, the issue cuts pretty deep. We have multiple potential inconsistencies when dealing with |
This reverts commit be61436.
|
Too bad it seems pretty hard to walk on the thin line right now. I think it is better to tighten the type system around arrays first. |
|
Right, I think so too, the latest version hits more issues :( |
|
@merykitty this pull request can not be integrated into git checkout ciRefArray
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push |
|
I think this PR is ready for review again. |
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me otherwise. Thanks!
src/hotspot/share/opto/type.cpp
Outdated
| } | ||
| _is_ptr_to_narrowoop = UseCompressedOops && ::is_reference_type(field_bt); | ||
| } else if (klass()->is_obj_array_klass()) { | ||
| _is_ptr_to_narrowoop = UseCompressedOops; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseCompressedOops is always true here, right? It's checked in line 3685. Also above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I have removed those check.
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
|
Thanks a lot for your reviews! /integrate |
|
Going to push as commit 9aff0ae. |
|
@merykitty Pushed as commit 9aff0ae. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
This PR splits
ciObjArrayKlassintociRefArrayKlassandciFlatArrayKlass, aligns the hierarchy with the corresponding types of the VM.Please kindly review, thanks a lot.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1739/head:pull/1739$ git checkout pull/1739Update a local copy of the PR:
$ git checkout pull/1739$ git pull https://git.openjdk.org/valhalla.git pull/1739/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1739View PR using the GUI difftool:
$ git pr show -t 1739Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1739.diff
Using Webrev
Link to Webrev Comment