Skip to content

OffHeap Changes #7345

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
Merged
vijaysun-omr merged 3 commits into from
Jun 21, 2024
Merged

OffHeap Changes #7345

vijaysun-omr merged 3 commits into from
Jun 21, 2024

Conversation

@rmnattas
Copy link
Contributor

@rmnattas rmnattas commented May 23, 2024

Changes to support OffHeap

  • Add pinning array support for OffHeap by storing the pinning array in node extension.
  • Add OffHeap support in constrainAnyIntLoad for valuePropagation
  • Update LoopVersioner code to support OffHeap

@rmnattas rmnattas requested a review from vijaysun-omr as a code owner May 23, 2024 14:28
@rmnattas rmnattas marked this pull request as draft May 23, 2024 17:07
@rmnattas
Copy link
Contributor Author

rmnattas commented May 23, 2024

Failure in CI JitBuilder testing, looking into it

Assertion failed at /home/vsts/work/1/s/compiler/il/OMRNode.cpp:212: self()->hasSymbolReference() + self()->hasRegLoadStoreSymbolReference() + self()->hasBranchDestinationNode() + self()->hasBlock() + self()->hasArrayStride() + self()->hasPinningArrayPointer() + self()->hasDataType() <= 1
_unionPropertyA union is not disjoint for this node aloadi (0x55a674c83880):
has({SymbolReference, ...}, ..., DataType) = ({1,0},0,0,0,1,0)

@rmnattas rmnattas force-pushed the off-heap-merge2 branch 2 times, most recently from 5f77115 to 68eb4f7 Compare May 23, 2024 17:44
@rmnattas rmnattas marked this pull request as ready for review May 23, 2024 18:09
@rmnattas
Copy link
Contributor Author

rmnattas commented May 23, 2024

Fixed

vijaysun-omr
vijaysun-omr reviewed Jun 3, 2024
View reviewed changes
vijaysun-omr
vijaysun-omr reviewed Jun 3, 2024
View reviewed changes
@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Jun 3, 2024

For the loop versioner commit, I would like @jdmpapin to also review.

There is a typo in that loop versioner commit message : "to use the chilid of aloadi tree"

@rmnattas rmnattas force-pushed the off-heap-merge2 branch 4 times, most recently from 9b3ed63 to 64c208b Compare June 5, 2024 18:03
vijaysun-omr
vijaysun-omr reviewed Jun 5, 2024
View reviewed changes
@rmnattas
Copy link
Contributor Author

rmnattas commented Jun 7, 2024

Changes for enabling pinning array for all nodes causes a failure now, looking at it.
Also working on the comments (not sure of tree shapes myself as I didn't do the code but will understand and comment).

@rmnattas
Copy link
Contributor Author

rmnattas commented Jun 11, 2024

I updated the VPHandler code and added comments, please review changes.
Also added a i2l check for the stride==1 case which didn't exist originally but I believe is needed.

@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Jun 11, 2024

The additional comments and changes help. The code looks somewhat easier to follow. I'll wait for Devin's review as well

@rmnattas rmnattas mentioned this pull request Jun 14, 2024
Open
jdmpapin
jdmpapin reviewed Jun 21, 2024
View reviewed changes
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.

Loop versioner changes LGTM, but there may be a small piece missing. I also have a couple of comments about findArrayIndexNode()

@rmnattas rmnattas mentioned this pull request Jun 21, 2024
Merged
VermaSh added 3 commits June 21, 2024 15:28
@VermaSh @rmnattas
Teach constrainAnyIntLoad in valuePropagation about dataAddrPointer
bb0b3e0 
Also remove unused findArrayIndexNode from VP constrainAddressRef

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
@VermaSh @rmnattas
LoopVersioner changes for OffHeap support
a0acc26 
Teaches LoopVersioner about dataAddr loads

Whenever we see a dataAddrPointer; all tests gets generated,
for null and array type, to use the child of the aloadi tree.

Fix `depsForLoopEntryPrep` tree pattern matching to allow for array
accesses to the 1st element using the dataAddrPointer, as that case
does not have an offset tree.

Fixing `requiresPrivatization()` to return false for dataAddr
nodes as they are internal-pointers.

Getting `childInRequiredForm` for the offheap case accessed the
wrong node with an extra `getFirstChild()` at the end.

Signed-off-by: Abdulrahman Alattas <rmnattas@gmail.com>
@VermaSh @rmnattas
Add pinning array pointer support for dataAddr nodes
4a10457 
Use node extentions to store pinning array pointer for non add nodes.

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Jun 21, 2024

Jenkins build all

@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Jun 21, 2024

Checks have passed. Merging.

@vijaysun-omr vijaysun-omr merged commit d8253e6 into eclipse-omr:master Jun 21, 2024
This was referenced Jul 10, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vijaysun-omr vijaysun-omr vijaysun-omr left review comments

@jdmpapin jdmpapin jdmpapin approved these changes

@babsingh babsingh Awaiting requested review from babsingh

@mstoodle mstoodle Awaiting requested review from mstoodle

@fjeremic fjeremic Awaiting requested review from fjeremic

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl

@knn-k knn-k Awaiting requested review from knn-k

@dsouzai dsouzai Awaiting requested review from dsouzai

+1 more reviewer

@VermaSh VermaSh VermaSh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

comp:compiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rmnattas @vijaysun-omr @VermaSh @jdmpapin