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

Update _unionPropertyA assert #19856

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Update _unionPropertyA assert #19856

merged 1 commit into from
Jul 18, 2024

Conversation

cjjdespres
Copy link
Contributor

The corresponding assert in omr/compiler/il/OMRNode.cpp was updated in the same way as this PR in eclipse-omr/omr#7345.

Fixes: #19836

@cjjdespres
Copy link
Contributor Author

Attn @mpirvu. Someone more familiar than me with the compiler IL should probably confirm that this is correct, but this seems right, given the OMR change.

mpirvu
mpirvu approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM.
@hzongaro Since this is outside my area of expertise, could you please review this PR too? Thanks

hzongaro
hzongaro approved these changes Jul 17, 2024
View reviewed changes
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 this change looks correct. Thanks!

@hzongaro
Copy link
Member

Jenkins test sanity all jdk8,jdk17,jdk21

@hzongaro hzongaro self-assigned this Jul 17, 2024
@hzongaro
Copy link
Member

After approving the change, I realized that I hadn't looked at the commit comments. May I ask you to add a little more detail to the commit? Maybe something like this?

If hasPinningArrayPointer() is true for the Node, but the Node is using the node extension to record the array pointer (supportsPinningArrayPointerInNodeExtension() is true), then it's not using the UnionPropertyA field to record that information. In that case, it's not a potentially conflicting use of the _unionPropertyA field.

@cjjdespres
Update _unionPropertyA assert
04930aa 
If hasPinningArrayPointer() is true for the Node, but the Node is using
the node extension to record the array pointer
(supportsPinningArrayPointerInNodeExtension() is true), then it's not
using the UnionPropertyA field to record that information. In that case,
it's not a potentially conflicting use of the _unionPropertyA field.

Signed-off-by: Christian Despres <[email protected]>
@cjjdespres
Copy link
Contributor Author

Sure, that message seems fine.

@hzongaro
Copy link
Member

Restarting testing:

Jenkins test sanity all jdk8,jdk17,jdk21

@hzongaro
Copy link
Member

JDK 8 test failure appears to be due to issue #18599.

Windows testing is out of commission at the moment, so we can ignore those.

Restarting x86 Linux JDK 17.

Jenkins test sanity xlinux jdk17

@cjjdespres cjjdespres mentioned this pull request Jul 18, 2024
Open
@hzongaro hzongaro merged commit 679e183 into eclipse-openj9:master Jul 18, 2024
36 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpirvu mpirvu mpirvu approved these changes

@hzongaro hzongaro hzongaro approved these changes

Assignees

@hzongaro hzongaro

Labels
comp:jit
Projects
None yet
Milestone
No milestone
3 participants
@cjjdespres @hzongaro @mpirvu