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

Remove remaining aconst_init, withfield, vnew refs #19812

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Remove remaining aconst_init, withfield, vnew refs #19812

merged 2 commits into from
Jul 8, 2024

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Jul 4, 2024 •
edited
Loading

Part 1: #19771

Fixes: #18316

@theresa-m theresa-m requested review from hangshao0 and a7ehuo July 4, 2024 17:50
@theresa-m theresa-m added comp:vm comp:jit project:valhalla Used to track Project Valhalla related work labels Jul 4, 2024
hzongaro
hzongaro reviewed Jul 4, 2024
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I took a quick look at the compiler changes. Thank you for making those changes, @theresa-m!

I had a draft pull request #18790 that had changes for allowing value type instances to be created either with new or with aconst_init and withfield, under the assumption that there might be a period of time where we would need to support both possibilities. Most of the changes there are irrelevant if we're dropping those two bytecode instructions in one step. However, I the think the change to TR_J9ByteCodeIlGenerator::genNew that calls setIdentityless and the change to TR_EscapeAnalysis::isImmutableObject will still be needed. May I ask you to pick up those two changes?

I'll also have to revise OMR pull request eclipse-omr/omr#7239 for the change to VPHandlers.cpp.

@theresa-m theresa-m requested a review from dsouzai as a code owner July 5, 2024 13:47
@theresa-m
Copy link
Contributor Author

@hzongaro I added your changes from #18790. Some of the changes were removed through merge conflicts and I also removed the areValueTypeInstancesCreatedWithBCNew method. Can you please review to make sure this is what you were expecting?

@hzongaro
Copy link
Member

hzongaro commented Jul 5, 2024

Some of the changes were removed through merge conflicts and I also removed the areValueTypeInstancesCreatedWithBCNew method. Can you please review to make sure this is what you were expecting?

Yes, that's what I looking for. Thanks for taking care of that! (Please squash that pair of commits - we don't need to preserve areValueTypeInstancesCreatedWithBCNew in the history.)

@hzongaro hzongaro mentioned this pull request Jul 5, 2024
Closed
@theresa-m
Copy link
Contributor Author

Thanks @hzongaro I squashed the commits.

a7ehuo
a7ehuo reviewed Jul 5, 2024
View reviewed changes
Copy link
Contributor

@a7ehuo a7ehuo left a comment

Choose a reason for hiding this comment

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

Thank you very much for the update @theresa-m! The JIT change looks good to me. I just have one minor comment.

hzongaro
hzongaro reviewed Jul 5, 2024
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.

May I ask you revise the commit message for commit e3a18a4 to remove the following paragraph, which no longer applies?

A new J9::ObjectModel::areValueTypeInstancesCreatedWithBCNew method is
used to enable creation of value type instances with the new bytecode.

theresa-m and others added 2 commits July 5, 2024 12:05
@theresa-m
Remove remaining aconst_init, withfield, vnew refs
ab95037 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@hzongaro @theresa-m
Prepare for value objects to be created with new bytecode
c00260e 
This change sets up changes to the JIT compiler that are needed to
permit value objects to be created with the new bytecode.  Previously
they could only be created with the aconst_init and withfield bytecode
instructions.  The new bytecode will be translated to the TR::New opcode
in IL, but existing optimizations that create value type instances will
continue to use the TR::newvalue opcode, so optimizations need to be
prepared for both possibilities for now.

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Copy link
Contributor Author

theresa-m commented Jul 5, 2024 •
edited
Loading

Thank you very much for the update @theresa-m! The JIT change looks good to me. I just have one minor comment.

Hey I tried removing these again and it looks like it compiles. Maybe I got confused with another spot :/ . They are removed now.

@theresa-m
Copy link
Contributor Author

May I ask you revise the commit message for commit e3a18a4 to remove the following paragraph, which no longer applies?

A new J9::ObjectModel::areValueTypeInstancesCreatedWithBCNew method is
used to enable creation of value type instances with the new bytecode.

I removed that paragraph. Is the part about using TR::newvalue still accurate?

@hzongaro
Copy link
Member

hzongaro commented Jul 5, 2024

Is the part about using TR::newvalue still accurate?

Yes - some optimizations still create value type instances with TR::newvalue.

hzongaro
hzongaro approved these changes Jul 5, 2024
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.

Compiler changes look good. Thanks!

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinuxval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended amac jdk21

hangshao0
hangshao0 approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM.

@hangshao0 hangshao0 merged commit 677181c into eclipse-openj9:master Jul 8, 2024
9 checks passed
@theresa-m theresa-m deleted the fix_18316_2 branch October 11, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangshao0 hangshao0 hangshao0 approved these changes

@hzongaro hzongaro hzongaro approved these changes

@a7ehuo a7ehuo a7ehuo approved these changes

@dsouzai dsouzai Awaiting requested review from dsouzai dsouzai is a code owner

Assignees
No one assigned
Labels
comp:jit comp:vm 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.

Remove bytecodes aconst_init, withfield and <vnew> method from Valhalla
4 participants
@theresa-m @hzongaro @hangshao0 @a7ehuo