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

Off Heap Changes and lowering trees bug #7296

Merged
merged 5 commits into from
May 17, 2024
Merged

Off Heap Changes and lowering trees bug #7296

merged 5 commits into from
May 17, 2024

Conversation

rmnattas
Copy link
Contributor

@rmnattas rmnattas commented Mar 27, 2024 •
edited

A number of off-heap commits and a fix to lower trees bug on Z

Commit 5fea995 copy the implementation of array access APIs from Openj9. Will be removed from OpenJ9 later.

@rmnattas rmnattas force-pushed the off-heap-merge1 branch 2 times, most recently from 79f0ee2 to 1f7f5b8 Compare March 27, 2024 16:40
@rmnattas
Copy link
Contributor Author

@VermaSh The commit c9309f7 fails in the CI testing because its only defined in OpenJ9 and OMR alone don't have it. Either their implementation should be moved to OMR and removed from Openj9 (which to me seem like the better way) or keeping the old implementation and only call to the TransformationUtil methods under #ifdef J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION

@VermaSh
Copy link
Contributor

VermaSh commented Mar 27, 2024

I agree, moving the implementation to OMR sounds like the better option.

@rmnattas
Copy link
Contributor Author

rmnattas commented Mar 28, 2024 •
edited

One test failure, the commit eclipse-openj9/openj9@72519a2 needs to be merged first to get the correct result when calling if (!comp()->generateArraylets()) when in balanced but off-heap is disabled.

VermaSh
VermaSh reviewed Apr 4, 2024
View reviewed changes
compiler/optimizer/OMRTransformUtil.cpp Outdated Show resolved Hide resolved
compiler/optimizer/OMRTransformUtil.cpp Outdated Show resolved Hide resolved
compiler/optimizer/OMRTransformUtil.hpp Outdated Show resolved Hide resolved
@VermaSh @rmnattas
OffHeap: Disable EA and idiom recognition till fixed for OffHeap
b19d6c7 
Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
@rmnattas
Copy link
Contributor Author

ValuePropagationCommon commit for off-heap was updated to continue supporting arraylet run environment.

The new commit is based on rmnattas@ed01b6d
Instead of modifying the code to support off-heap which breaks runs that uses arraylets, the commit now adds separate paths for off-heap while keeping the original paths as-is.

@rmnattas
Copy link
Contributor Author

PR is ready for review

@rmnattas rmnattas marked this pull request as ready for review April 23, 2024 13:59
@rmnattas rmnattas mentioned this pull request Apr 26, 2024
Open
@rmnattas rmnattas mentioned this pull request May 10, 2024
Open
@rmnattas
Copy link
Contributor Author

PR ready for merge

0xdaryl
0xdaryl reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

In addition to the comments below, the commit comment for b6bd114 should be more descriptive of the problem and what was fixed.

compiler/z/codegen/OMRCodeGenerator.cpp Show resolved Hide resolved
compiler/optimizer/OMRTransformUtil.hpp Outdated Show resolved Hide resolved
compiler/optimizer/OMRTransformUtil.hpp Outdated Show resolved Hide resolved
compiler/optimizer/OMRTransformUtil.hpp Outdated Show resolved Hide resolved
@VermaSh @rmnattas
Fix bug in lowering trees marked as internal pointer
e29b0ae 
Keep the internal pointer flag for the new nodes when
lowering trees.

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

Agree name changing requested above will make it easier to understand.
The names in OMR implementation and definitions will be updated in this PR. For OpenJ9 only definition is merged so far and will be removed. Work in progress and upcoming PRs need to update naming accordingly.

@rmnattas
Move APIs from OpenJ9 to OMR to help with array access
0aaf99b 
APIs are:
`generateDataAddrLoadTrees`
`generateArrayAddressTrees` named `generateArrayElementAddressTrees`
`generateArrayStartTrees` named `generateFirstArrayElementAddressTrees`
`generateArrayOffsetTrees` named `generateConvertArrayElementIndexToOffsetTrees`

OpenJ9 commit to revert: eclipse-openj9/openj9@e766755

Signed-off-by: Abdulrahman Alattas <rmnattas@gmail.com>

name
@0xdaryl 0xdaryl self-assigned this May 14, 2024
@0xdaryl
Copy link
Contributor

0xdaryl commented May 14, 2024

See the Windows build failure:

D:\a\1\s\compiler\optimizer\OMRTransformUtil.cpp(485,91): error C2220: the following warning is treated as an error [D:\a\1\s\build\fvtest\compilertest\testcompiler.vcxproj]
D:\a\1\s\compiler\optimizer\OMRTransformUtil.cpp(485,91): warning C4244: 'argument': conversion from 'uintptr_t' to 'int32_t', possible loss of data [D:\a\1\s\build\fvtest\compilertest\testcompiler.vcxproj]
  OMROptimizer.cpp

rmnattas and others added 2 commits May 15, 2024 09:23
@rmnattas
Update Array helper APIs in ValuePropogationCommon for OffHeap
644c1ce 
This commit is based on ed01b6d
Instead of modifying the code to support off-heap which as-is breaks
runs with arraylet environments, this commit adds separate paths for
off-heap while keeping the original paths as-is.

Code can be modified similar to the based-on commit to have gencon
runs using the array helper APIs while not affecting arraylet runs.

Signed-off-by: Abdulrahman Alattas <rmnattas@gmail.com>
@VermaSh @rmnattas
Treat contiguousArrayDataAddrFieldSymbol as Int64
a6e711a 
dataAddr field width is always 64 bit so treating it as TR::Address results in
null being loaded from top 32 bits in compressed refs. We need the symbol to be
TR::Int64 for it be loaded correctly.

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

Fixed casting issue 👍

@0xdaryl
Copy link
Contributor

0xdaryl commented May 15, 2024

Jenkins build all

@0xdaryl
Copy link
Contributor

0xdaryl commented May 16, 2024

@LinHu2016 @VermaSh : have your comments been addressed? Please approve if so.

@0xdaryl 0xdaryl merged commit dd4c5d5 into eclipse:master May 17, 2024
15 of 18 checks passed
rmnattas added a commit to rmnattas/omr that referenced this pull request Jun 4, 2024
@rmnattas
Revert "Merge pull request eclipse#7296 from rmnattas/off-heap-merge1"
456db0a 
This reverts commit dd4c5d5, reversing
changes made to 76296e1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LinHu2016 LinHu2016 LinHu2016 left review comments

@VermaSh VermaSh VermaSh approved these changes

@0xdaryl 0xdaryl 0xdaryl approved these changes

@fjeremic fjeremic Awaiting requested review from fjeremic fjeremic is a code owner

@vijaysun-omr vijaysun-omr Awaiting requested review from vijaysun-omr vijaysun-omr is a code owner

@dsouzai dsouzai Awaiting requested review from dsouzai dsouzai is a code owner

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

Assignees

@0xdaryl 0xdaryl

Labels
arch:z comp:compiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rmnattas @VermaSh @0xdaryl @LinHu2016