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 reference array element alignment test for stable arrays #15512

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

gita-omr
Copy link
Contributor

@gita-omr gita-omr commented Jul 11, 2022

  • we would spuriously fail to fold loads of reference array
    elements at odd positions when compressed references were enabled
    because the alignment test looked for 8-byte alignment even though
    only 4-byte alignment is really required

Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

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

Code changes LGTM with one mild suggestion.

Another suggestion about the message:

take compressed references in account when checking if array element is null

For most types, the alignment check is there just to make sure that we're looking at exactly one array element when we try to determine whether the value is zero, but as I mentioned in my other comment, a misaligned reference load isn't even safe to perform at all. So I think it would make more sense to characterize this in the description/commit message as a fix for the reference array element alignment test. It does also ensure that we're looking at exactly one array element for the purpose of checking for a null though, so I'll leave it to you how best to describe it

edited to add: IMO it would be good at least to spell out the problem that this fixes (which I assume is the following): we would spuriously fail to fold loads of reference array elements at odd positions when compressed references were enabled because the alignment test looked for 8-byte alignment even though only 4-byte alignment is really required

runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
- we would spuriously fail to fold loads of reference array
elements at odd positions when compressed references were enabled
because the alignment test looked for 8-byte alignment even though
only 4-byte alignment is really required
@jdmpapin
Copy link
Contributor

Jenkins test sanity all jdk17

@jdmpapin
Copy link
Contributor

@vijaysun-omr, has your question been adequately addressed? And if so, do you have anything further, or does the change look OK to you? (subject to the PR testing, of course)

@gita-omr gita-omr changed the title Improve stable array element folding Fix reference array element alignment test for stable arrays Jul 13, 2022
@vijaysun-omr
Copy link
Contributor

Yes, thanks, I have approved the change now.

@jdmpapin jdmpapin merged commit 8ead00a into eclipse-openj9:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants