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

Z: Zero multi array size when batch clear is disabled. #19557

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Z: Zero multi array size when batch clear is disabled. #19557

merged 1 commit into from
Jun 21, 2024

Conversation

ehsankianifar
Copy link
Contributor

We inline the multi array allocation when the size of first or second dimension is zero but the instruction don't set the zero size filed of the new object. This is not a problem when we allocate from zeroed memory but if the memory is not zeroed, it causes problems.
In this PR I add instructions to explicitly set size filed to zero.

@ehsankianifar ehsankianifar marked this pull request as draft May 27, 2024 15:09
@ehsankianifar ehsankianifar force-pushed the Z_FixMultiANewArrayEvaluator branch 2 times, most recently from ec864bb to 601701b Compare May 27, 2024 16:58
@ehsankianifar ehsankianifar marked this pull request as ready for review May 27, 2024 17:23
@ehsankianifar
Copy link
Contributor Author

@r30shah this PR is ready for review. I confirm this fixes the issue with MultiANewArray tree.

r30shah
r30shah requested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Can you share the sequence generated now (Instruction selection log). Also I would like to understand the purpose behind setting the discontiguous size field to 0 (but not mustBeZero field), is it a bug / some clever seq?

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
@ehsankianifar ehsankianifar marked this pull request as draft May 28, 2024 21:58
@ehsankianifar
Z: Zero multi array size when batch clear is disabled.
06ec759 
We inline multi array allocation when dim 1 or 2 are zero.
When allocate from zeroed memory, size field is already zero.
When batch clear is disabled, we must explicitly zero the size filed.
This commit add instructions to zero the size filed.

Signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>
@ehsankianifar
Copy link
Contributor Author

Discontiguous layout is used for all zero size arrays. In discontiguous array layout, there should always be zero on the offset that the size field of contiguous array is located and we distinguish discontiguous arrays from contiguous arrays by looking at the value of size field as if the array is contiguous, if it is zero the array is discontiguous otherwise it is contiguous.
This is the reason we should always set the offset of size field to zero for both contiguous and discontiguous array when the array length is zero. I added a few comments to make it more clear on the code.

@ehsankianifar ehsankianifar marked this pull request as ready for review May 30, 2024 18:31
@ehsankianifar
Copy link
Contributor Author

@r30shah this PR is ready for review.

r30shah
r30shah approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

@r30shah
Copy link
Contributor

r30shah commented Jun 10, 2024

jenkins test sanity zlinux jdk11,jdk21

@ehsankianifar
Copy link
Contributor Author

@r30shah is it okay if we merge this PR?

@r30shah
Copy link
Contributor

r30shah commented Jun 21, 2024

jenkins test sanity zlinux jdk17

@r30shah
Copy link
Contributor

r30shah commented Jun 21, 2024

I will merge this once JDK17 test finishes

@r30shah r30shah merged commit 387aa4c into eclipse-openj9:master Jun 21, 2024
9 checks passed
@ehsankianifar ehsankianifar deleted the Z_FixMultiANewArrayEvaluator branch July 15, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ehsankianifar @r30shah