Skip to content

Load default value instance if the class is initialized - #15666

Merged
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
a7ehuo:static-allocation-defaultvalue-aot-3
Aug 12, 2022
Merged

Load default value instance if the class is initialized#15666
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
a7ehuo:static-allocation-defaultvalue-aot-3

Conversation

@a7ehuo

@a7ehuo a7ehuo commented Aug 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

a7ehuo commented Aug 4, 2022

Copy link
Copy Markdown
Contributor Author

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

@a7ehuo

a7ehuo commented Aug 4, 2022

Copy link
Copy Markdown
Contributor Author

@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

@dsouzai dsouzai left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread runtime/compiler/runtime/RelocationRecord.cpp Outdated
hzongaro
hzongaro reviewed Aug 8, 2022
View reviewed changes

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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.

Comment thread runtime/compiler/runtime/RelocationRecord.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
@a7ehuo
a7ehuo force-pushed the static-allocation-defaultvalue-aot-3 branch from 1353049 to 013aa3e Compare August 8, 2022 22:14
@a7ehuo

a7ehuo commented Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

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

dsouzai
dsouzai requested changes Aug 9, 2022
View reviewed changes

@dsouzai dsouzai left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread runtime/compiler/runtime/RelocationRecord.cpp Outdated
Annabelle Huo Annabelle Huo
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

a7ehuo commented Aug 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

dsouzai commented Aug 11, 2022

Copy link
Copy Markdown
Contributor

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

@hzongaro hzongaro self-assigned this Aug 11, 2022
@hzongaro

hzongaro commented Aug 11, 2022

Copy link
Copy Markdown
Member

Jenkins test sanity all jdk17

@a7ehuo

a7ehuo commented Aug 11, 2022

Copy link
Copy Markdown
Contributor Author

@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

hzongaro commented Aug 12, 2022

Copy link
Copy Markdown
Member

Jenkins test sanity,extended xlinuxval jdknext

@hzongaro

hzongaro commented Aug 12, 2022

Copy link
Copy Markdown
Member

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

@hzongaro

hzongaro commented Aug 12, 2022

Copy link
Copy Markdown
Member

Jenkins test sanity+aot xlinux jdknext

@hzongaro

hzongaro commented Aug 12, 2022

Copy link
Copy Markdown
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

@dsouzai dsouzai dsouzai approved these changes
@hzongaro hzongaro hzongaro 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