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 sequentialStoreSimplifier for OffHeap #20826

Closed
wants to merge 1 commit into from

Conversation

rmnattas
Copy link
Contributor

SequentialStoreSimplifier checks the baseVariable symRef to confirm compatibility. With OffHeap baseVariable used is the dataAddrPtr which would match symRefs even for different arrays.

This fix compares the dataAddrPtr child symRefs for compatibility.

SequentialStoreSimplifier checks the baseVariable symRef to confirm
compatibility. With OffHeap baseVariable used is the dataAddrPtr
which would match symRefs even for different arrays.

This commit compares the dataAddrPtr child symRef for
compatibility.
@rmnattas
Copy link
Contributor Author

fyi @zl-wang @hzongaro
Testing PR with OffHeap enabled.

baseNode = baseNode->getFirstChild();
activeBaseRef = baseNode->getSymbolReference();
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to compare every step of getting active/base ref? currently, you only compare the end results, ignoring if the middle-step(s) are consistent as well. i.e. can there be a situation where one of the refs is gotten without going through dataAddr node ... tree shapes are different, but ref(s) are the same?

Copy link
Contributor Author

@rmnattas rmnattas Dec 12, 2024

Choose a reason for hiding this comment

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

The method generateArraycopyFromSequentialStores is called for a store and it does checks (the store node, the value node, the dest node) for subsequent trees for valid candidates, and I'm changing where it checks the base address symRef in the dest node check. One other shape that's introduced with OffHeap is if its accessing the first element in an array where there's no aladd. Other modifications needed to handle that.

arr[0]
n60n        aloadi  <contiguousArrayDataAddrField>[#373  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [0x767885060280] bci=[-1,46,2542] rc=1 vc=426 vn=- li=- udi=- nc=1 flg=0x80>
n11n          ==>newarray

vs

arr[1] // byte-array
n62n        aladd (X>=0 internalPtr )                                                         [0x767885060320] bci=[-1,46,2542] rc=2 vc=426 vn=- li=- udi=- nc=2 flg=0x8100
n60n          aloadi  <contiguousArrayDataAddrField>[#373  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [0x767885060280] bci=[-1,46,2542] rc=1 vc=426 vn=- li=- udi=- nc=1 flg=0x80>
n11n            ==>newarray
n61n          lconst 1 (highWordZero X!=0 X>=0 )                                              [0x7678850602d0] bci=[-1,46,2542] rc=1 vc=426 vn=- li=- udi=- nc=0 flg=0x4104

@rmnattas
Copy link
Contributor Author

rmnattas commented Dec 16, 2024

Closing issue, moving fixes to:

  1. OffHeap tree shape identification in [OMR]
    Use dataAddrPtr child as the baseVar when processing TR_AddressTree eclipse-omr/omr#7593
  2. sequentialStoreSimplifier symRef check bug fix in
    Compare nodes in addition to symRef in SequentialStoreSimplification #20841

@rmnattas rmnattas closed this Dec 16, 2024
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.

2 participants