-
Notifications
You must be signed in to change notification settings - Fork 143
8375548: [lworld] C2: Support value class arrays with compiler replay #1931
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
base: lworld
Are you sure you want to change the base?
Conversation
…not loading object array klasses with ArrayProperties::DEFAULT
|
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
src/hotspot/share/ci/ciReplay.cpp
Outdated
| if (Arguments::is_valhalla_enabled() && k->is_objArray_klass()) { | ||
| // Create ref or flat array klass. | ||
| k = ObjArrayKlass::cast(k)->klass_with_properties(ArrayKlass::ArrayProperties::DEFAULT, THREAD); | ||
| } |
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.
This will always create the default refined klass. Shouldn't we keep track of which refined klass was actually referenced in the replay file and then create that particular one here?
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.
Good point! I reverted this fix and added proper support for value class arrays by dumping the array properties to the replay file. I've also added some more extended testing.
… due to not loading object array klasses with ArrayProperties::DEFAULT" This reverts commit 0cdb254.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. | |||
| * Copyright (c) 2021, 2026, Oracle and/or its affiliates. All rights reserved. | |||
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.
I've shared some code with this class which already did some of the things required for the new tests.
When using compiler replay with
--enable-previewwith array store/load profiling, we only createObjArrayKlassobjects instead of subclass objects (i.e.RefArrayKlassorFlatArrayKlass). The reason is that we are directly resolving klasses withSystemDirectory::resolve_or_fail():valhalla/src/hotspot/share/ci/ciReplay.cpp
Line 557 in a4fb7eb
This method will call
InstanceKlass::array_klass()at some point which directly creates anObjArrayKlassobject. This let's the replayed compilation fail when trying to speculate on an value class array which expects a subclass ofObjArrayKlass.The fix I propose is to make sure that we are always creating subclass objects by explicitly using
ArrayProperties::DEFAULTwhen parsing anArrayKlassduring replay compilation.I added a compiler replay test which triggers the same assert as shown in the report.
Thanks,
Christian
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1931/head:pull/1931$ git checkout pull/1931Update a local copy of the PR:
$ git checkout pull/1931$ git pull https://git.openjdk.org/valhalla.git pull/1931/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1931View PR using the GUI difftool:
$ git pr show -t 1931Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1931.diff
Using Webrev
Link to Webrev Comment