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.

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

Fix null restricted array related issues for value types #7452

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Sep 4, 2024

(1) Add front end getNullRestrictedArrayClassFromComponentClass to retrieve null restricted array class
(2) Rename isArrayCompTypePrimitiveValueType to isArrayNullRestricted to reflect the updated logic
(3) Fix array load in transformNullRestrictedArrayCopy

Depends on

Related: eclipse-openj9/openj9#19914, eclipse-openj9/openj9#19913

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 4, 2024

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

This PR is created as a draft and marked as WIP for now because of all the dependencies. Regarding to the implementation itself, it is ready for review.

Since this PR depends on eclipse-openj9/openj9#20112, it should be review along with eclipse-openj9/openj9#20112.

@hzongaro hzongaro self-assigned this Sep 4, 2024
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.

Looking at these changes in isolation, I think they look good. Just a few very minor comments. I'll come back again after reviewing OpenJ9 pull request eclipse-openj9/openj9#20112.

Also, may I ask you to expand on the description for commit 6e2377c to describe what getNullRestrictedArrayClassFromComponentClass is expected to do, and also the description for commit 0353738 to indicate why the name change is needed?

compiler/env/FrontEnd.hpp Show resolved Hide resolved
compiler/optimizer/OMRValuePropagation.hpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
@hzongaro
Copy link
Member

hzongaro commented Sep 6, 2024

May I also ask you to remove the Markdown from the commit comments?

Add front end getNullRestrictedArrayClassFromComponentClass
that retrieves the nullRestrictedArrayClass pointer from the
array component class if it exist.

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from 8075e57 to a697231 Compare October 8, 2024 15:21
@a7ehuo a7ehuo marked this pull request as ready for review October 8, 2024 15:21
@a7ehuo a7ehuo changed the title WIP: Fix null restricted array related issues for value types Fix null restricted array related issues for value types Oct 8, 2024
Rename isArrayCompTypePrimitiveValueType
to isArrayNullRestricted. Whether or not an array is
null restricted is no longer decided by array component
type but by the J9ClassArrayIsNullRestricted flag in
array class.

Signed-off-by: Annabelle Huo <[email protected]>
Load the source array and the destination array from
the saved temp instead of commoning the original nodes
incorrectly across blocks.

Related: eclipse-openj9/openj9#19913, eclipse-openj9/openj9#19914

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from a697231 to 833607b Compare October 8, 2024 15:28
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Oct 8, 2024

@hzongaro All comments have been addressed in the latest commits. Ready for another review. Thank you!

Please note that I have rebased this PR to the latest master branch and this PR requires coordinated merge with eclipse-openj9/openj9#20112.

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. I will wait for eclipse-openj9/openj9#20112 to be ready before I merge this pull request.

@hzongaro
Copy link
Member

Jenkins build all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants