Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Load default value instance if the class is initialized #15666

Merged
merged 1 commit into from
Aug 12, 2022
Merged

Load default value instance if the class is initialized #15666

merged 1 commit into from
Aug 12, 2022

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Aug 4, 2022 •
edited
Loading

ILGen:

  • aconst_init: Load the default value instance if the class is initialized. This enhancement can be disabled
    by setting env variable TR_DisableLoadStaticDefaultValueInstance.

SymRef:

  • Create symbol reference for default value instance of value class.

AOT (X86):

  • Add new relocation record TR_RelocationRecordStaticDefaultValueInstance
    to materialize the default value instance slot address on AOT load.

Depends on

Signed-off-by: Annabelle Huo Annabelle.Huo@ibm.com

@a7ehuo a7ehuo added comp:jit project:valhalla Used to track Project Valhalla related work labels Aug 4, 2022
@a7ehuo a7ehuo requested review from dsouzai and hzongaro August 4, 2022 18:00
@a7ehuo a7ehuo mentioned this pull request Aug 4, 2022
Merged
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 4, 2022

This PR has dependency on eclipse-omr/omr#6641 and should be reviewed together with it

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 4, 2022

@dsouzai @hzongaro May I ask you to review this change along with eclipse-omr/omr#6641? Thank you!

dsouzai
dsouzai requested changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me; minor change requested.

runtime/compiler/runtime/RelocationRecord.cpp Outdated Show resolved Hide resolved
hzongaro
hzongaro reviewed Aug 8, 2022
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a couple of minor suggestions.

runtime/compiler/runtime/RelocationRecord.cpp Outdated Show resolved Hide resolved
runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved
@a7ehuo a7ehuo force-pushed the static-allocation-defaultvalue-aot-3 branch from 1353049 to 013aa3e Compare August 8, 2022 22:14
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 8, 2022

@hzongaro @dsouzai All comments are addressed. Ready for another review. Thanks!

dsouzai
dsouzai requested changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this one issue earlier so requested the change now.

runtime/compiler/runtime/RelocationRecord.cpp Outdated Show resolved Hide resolved
@a7ehuo
Load default value instance if the class is initialized
38b00cb 
ILGen:
- aconst_init: Load the default value instance if the
class is initialized. This enhancement can be disabled
by setting env variable `TR_DisableLoadStaticDefaultValueInstance`.

SymRef:
- Create symbol reference for default value instance
of value class.

AOT (X86):
- Add new relocation record `TR_RelocationRecordStaticDefaultValueInstance`
to materialize the default value instance slot address
on AOT load.

Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
@a7ehuo a7ehuo force-pushed the static-allocation-defaultvalue-aot-3 branch from 013aa3e to 38b00cb Compare August 9, 2022 20:21
@a7ehuo a7ehuo changed the title WIP: Load default value instance if the class is initialized Load default value instance if the class is initialized Aug 11, 2022
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 11, 2022 •
edited
Loading

Removed WIP. The OMR dependency eclipse-omr/omr#6641 has been merged to https://github.com/eclipse-openj9/openj9-omr. This PR is ready for another review and PR build test.

@dsouzai
Copy link
Contributor

dsouzai commented Aug 11, 2022

AOT changes look good to me. @hzongaro do you mind being committer for this one?

@hzongaro hzongaro self-assigned this Aug 11, 2022
@hzongaro
Copy link
Member

Jenkins test sanity all jdk17

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 11, 2022

@hzongaro For this PR, JDKNext standard and JDKNext valhalla should be tested as well. You probably already plan to do so. Just a reminder

@hzongaro
Copy link
Member

Jenkins test sanity,extended xlinuxval jdknext

@hzongaro
Copy link
Member

xlinuxval build failure appears to be unrelated to this pull request.

@hzongaro
Copy link
Member

Jenkins test sanity+aot xlinux jdknext

@hzongaro
Copy link
Member

Jenkins test sanity,extended xlinuxval jdknext

@hzongaro hzongaro merged commit cde425e into eclipse-openj9:master Aug 12, 2022
@a7ehuo a7ehuo mentioned this pull request Jan 30, 2023
Open
3 tasks
@a7ehuo a7ehuo deleted the static-allocation-defaultvalue-aot-3 branch February 7, 2023 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzongaro hzongaro hzongaro approved these changes

@dsouzai dsouzai dsouzai approved these changes

Assignees

@hzongaro hzongaro

Labels
comp:jit project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@a7ehuo @dsouzai @hzongaro