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

OffHeap Changes #7345

Merged
merged 3 commits into from
Jun 21, 2024
Merged

OffHeap Changes #7345

merged 3 commits into from
Jun 21, 2024

Conversation

rmnattas
Copy link
Contributor

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
Copy link
Contributor Author

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

Fixed

@vijaysun-omr
Copy link
Contributor

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
@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

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

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

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()

compiler/optimizer/VPHandlers.cpp Outdated Show resolved Hide resolved
compiler/optimizer/VPHandlers.cpp Outdated Show resolved Hide resolved
compiler/optimizer/LoopVersioner.cpp Show resolved Hide resolved
Also remove unused findArrayIndexNode from VP constrainAddressRef

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
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>
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

Jenkins build all

@vijaysun-omr
Copy link
Contributor

Checks have passed. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants