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 #20112

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

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Sep 4, 2024

(1) Add utility methods for null restricted arrays
(2) Fix null restricted array related issues

  • Add recognized method jdk/internal/value/ValueClass.newArrayInstance
  • Add VP fixed class constraint if ValueClass.newArrayInstance creates null restricted array
  • Rename isArrayCompTypePrimitiveValueType to isArrayNullRestricted to reflect the updated logic
  • Update isArrayNullRestricted on how to determine whether or not an array is a null restricted array
  • Disable transformation based on type hint which is no longer sufficient enough to decide whether or not the array is null restricted or not
  • If flattenable arrays are enabled, do not create fixed class constraint for parm array class based on signature since both nullable and null restricted arrays share the same signature

Depends on

Fixes: #19913, #19914, #19708

@a7ehuo a7ehuo added comp:jit project:valhalla Used to track Project Valhalla related work depends:omr Pull request is dependent on a corresponding change in OMR depends:omr:breaking labels Sep 4, 2024
@a7ehuo a7ehuo requested a review from hzongaro September 4, 2024 18:12
@a7ehuo a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch 2 times, most recently from c469e30 to 516a676 Compare September 4, 2024 18:21
@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/omr#7452, it should be review along with eclipse/omr#7452.

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.

I think these changes look good - just a few minor comments.

Could you expand on the reason for these changes in the commit comment? May I also ask you to remove the Markdown from the commit comment?

runtime/oti/j9.h Outdated Show resolved Hide resolved
runtime/compiler/env/VMJ9.h Show resolved Hide resolved
runtime/compiler/env/VMJ9.cpp Outdated Show resolved Hide resolved
runtime/compiler/env/VMJ9.cpp Outdated Show resolved Hide resolved
runtime/compiler/env/J9ClassEnv.cpp Show resolved Hide resolved
runtime/compiler/env/VMJ9Server.cpp Outdated Show resolved Hide resolved
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.

Sorry - I somehow neglected to review the Value Propagation changes in my first pass through this pull request

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
runtime/compiler/env/j9method.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
Whether or not an array is null-restricted is no longer
decided by array component type but by the
J9ClassArrayIsNullRestricted flag in array class.

Only the nullRestrictedArrayClass of the J9Class has
J9ClassArrayIsNullRestricted set.

(1)
Add getNullRestrictedArrayClassFromComponentClass
to retrieve nullRestrictedArrayClass from J9Class

(2)
Add isArrayNullRestricted to check if an array class
is null restricted by checking J9ClassArrayIsNullRestricted.

Signed-off-by: Annabelle Huo <[email protected]>
(1) Add recognized methods from jdk/internal/value/ValueClass

(2)
Add VP fixed class constraint if ValueClass.newArrayInstance
creates null restricted array using NullRestrictedCheckedType

(3)
Rename isArrayCompTypePrimitiveValueType
to isArrayNullRestricted to reflect the updated logic

Update isArrayNullRestricted on how to determine
whether or not an array is a null restricted array

(4)
Disable transformation based on type hint which is no longer
sufficient enough to decide whether or not the array is null
restricted or not

(5)
If flattenable arrays are enabled, do not create fixed class
constraint for parm array class based on signature since
both nullable and null restricted arrays share the same signature

(6)
Enable previous disabled JIT Valhalla functional tests

Fixes: eclipse-openj9#19913, eclipse-openj9#19914, eclipse-openj9#19708

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from bb24a35 to 3af4ff9 Compare October 8, 2024 15:33
@a7ehuo a7ehuo marked this pull request as ready for review October 8, 2024 15:34
@a7ehuo a7ehuo requested a review from dsouzai as a code owner October 8, 2024 15:34
@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
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Oct 8, 2024

@hzongaro All comments have been addressed in the latest commits, except for the following three that will be addressed in the future PRs due to the complexity. Ready for another review. Thank you!

(1) #20112 (comment)
(2) #20112 (comment)
(3) #20112 (comment)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit depends:omr:breaking depends:omr Pull request is dependent on a corresponding change in OMR project:valhalla Used to track Project Valhalla related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ValueTypeArrayTestsJIT test failures
3 participants