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

Check if an array class can be trusted as a fixed class #7579

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Check if an array class can be trusted as a fixed class #7579

merged 1 commit into from
Dec 20, 2024

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Dec 2, 2024 •
edited
Loading

There are cases where two different array classes could share the same signature in downstream projects. In such cases, the class retrieved from signature cannot be trusted as a fixed class.

Related: eclipse-openj9/openj9#20522

@a7ehuo a7ehuo requested a review from vijaysun-omr as a code owner December 2, 2024 14:49
@a7ehuo a7ehuo requested review from hzongaro and removed request for vijaysun-omr December 2, 2024 14:50
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 2, 2024

@hzongaro May I ask you to review this change? Thank you!

@a7ehuo a7ehuo changed the title Fix VP constraint for null-restricted array class Value Types: Fix VP constraint for null-restricted array class Dec 4, 2024
hzongaro
hzongaro reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@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.

I think the changes look correct, but I have a couple of thoughts about how things might be arranged differently.

compiler/optimizer/VPConstraint.cpp Outdated Show resolved Hide resolved
compiler/optimizer/VPConstraint.cpp Outdated Show resolved Hide resolved
@a7ehuo a7ehuo force-pushed the valuetype-fix-vp-constraint-for-nullRestrictedArray branch from c197ea8 to 2cb7621 Compare December 11, 2024 13:36
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 11, 2024

@hzongaro All comments are addressed. Ready for another review. Thank you!

hzongaro
hzongaro reviewed Dec 18, 2024
View reviewed changes
compiler/optimizer/VPConstraint.cpp Outdated Show resolved Hide resolved
compiler/optimizer/VPHandlers.cpp Outdated Show resolved Hide resolved
compiler/optimizer/VPHandlers.cpp Outdated Show resolved Hide resolved
compiler/optimizer/VPHandlers.cpp Outdated Show resolved Hide resolved
@a7ehuo a7ehuo force-pushed the valuetype-fix-vp-constraint-for-nullRestrictedArray branch from 2cb7621 to c51517f Compare December 18, 2024 16:18
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 18, 2024

@hzongaro All comments are addressed in c51517f. The downstream implementation is in eclipse-openj9/openj9#20853. Ready for another review. Thank you!

@a7ehuo a7ehuo mentioned this pull request Dec 18, 2024
Open
hzongaro
hzongaro approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@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.

I think these changes look good. Thanks!

@hzongaro
Copy link
Contributor

Jenkins build all

@hzongaro
Copy link
Contributor

Sorry. I just noticed that the commit comment still talks about null-restricted arrays. May I ask you to adjust the comment to speak more generally about how downstream projects might have different conditions under which array classes can be trusted to be fixed classes? Also, please remove the reference to the OpenJ9 pull request from the commit comment.

@a7ehuo
Check if an array class can be trusted as a fixed class
eb64424 
There are cases where two different array classes could share
the same signature in downstream projects. In such cases, the
class retrieved from signature cannot be trusted as a fixed class.

Related: eclipse-openj9/openj9#20522
Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the valuetype-fix-vp-constraint-for-nullRestrictedArray branch from c51517f to eb64424 Compare December 19, 2024 19:24
@a7ehuo a7ehuo changed the title Value Types: Fix VP constraint for null-restricted array class Check if an array class can be trusted as a fixed class Dec 19, 2024
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 19, 2024

... remove the reference to the OpenJ9 pull request from the commit comment.

@hzongaro This PR is one of the fixes for issue eclipse-openj9/openj9#20522. The reference in the commit message is to the issue eclipse-openj9/openj9#20522 not the PR eclipse-openj9/openj9#20853.

All comments are addressed. Ready for another review. Thank you!

@hzongaro
Copy link
Contributor

The reference in the commit message is to the issue. . . not the PR

Sorry for my confusion!

@hzongaro
Copy link
Contributor

Jenkins build all

@hzongaro
Copy link
Contributor

win, zos and linux_riscv64 problems are expected. Merging.

@hzongaro hzongaro merged commit 3ddaad3 into eclipse-omr:master Dec 20, 2024
10 of 13 checks passed
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

Assignees
No one assigned
Labels
comp:compiler
Projects
None yet
Milestone
No milestone
Development

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