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

Update System.arraycopy to support null restricted value type arrays #7084

Merged

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Aug 2, 2023

Required by JEP 401: If null restricted value type is enabled, we need to preform null check on the value being stored in order to throw a NullPointerException if the array is null-restricted and the value to write is null. If it is this case, System.arraycopy cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array are null restricted value type, and whether or not they are flattened, VP decides whether or not transform System.arraycopy.

Fixes: eclipse-openj9/openj9#17196, eclipse-openj9/openj9#17586

@a7ehuo a7ehuo requested review from hzongaro and removed request for mstoodle, 0xdaryl and vijaysun-omr August 2, 2023 19:38
@a7ehuo a7ehuo changed the title WIP: Update System.arraycopy to support null restricted value type arrays Update System.arraycopy to support null restricted value type arrays Aug 3, 2023
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 3, 2023

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

@a7ehuo a7ehuo force-pushed the system-arraycopy-primitive-value-types-PR branch from 0bdfa3f to 8bb98cf Compare August 3, 2023 13:21
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.

Looks great! Mostly minor comments

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/env/OMRObjectModel.hpp Outdated Show resolved Hide resolved
compiler/env/OMRClassEnv.hpp Show resolved Hide resolved
compiler/optimizer/OMRValuePropagation.cpp Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Show resolved Hide resolved
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 17, 2023

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

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 18, 2023

eclipse.omr x86-64 macOS test failed. It looks to be an known issue #6516

2023-08-17T19:42:57.2705020Z 32: [  FAILED  ] 2 tests, listed below:
2023-08-17T19:42:57.2705960Z 32: [  FAILED  ] PortSockTest.poll_functionality_basic
2023-08-17T19:42:57.2706840Z 32: [  FAILED  ] PortSockTest.poll_functionality_many_sockets
2023-08-17T19:42:57.2707870Z 32: 
2023-08-17T19:42:57.2708790Z 32:  2 FAILED TESTS
The following tests FAILED:
	 32 - porttest (Failed)

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.

A few minor comments, plus I think there might be another case where a run-time test for null-restricted component types is needed.

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the system-arraycopy-primitive-value-types-PR branch from d0bf0ce to 71ddb70 Compare August 22, 2023 13:29
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Aug 22, 2023

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

@hzongaro hzongaro self-assigned this Aug 22, 2023
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.

Looks good. Thanks!

@hzongaro
Copy link
Contributor

Jenkins build all

@hzongaro hzongaro merged commit 3a6206c into eclipse-omr:master Aug 22, 2023
@a7ehuo a7ehuo deleted the system-arraycopy-primitive-value-types-PR branch March 6, 2024 16:29
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.

JIT generated code must avoid using arraycopy instruction if a NullPointerException could result
2 participants