Skip to content

Adjust arguments to Unsafe.setMemory() on PPC when offheap is enabled - #7145

Merged
0xdaryl merged 5 commits into
eclipse-omr:masterfrom
midronij:offheap-unsafe-setmemory
Aug 28, 2024
Merged

Adjust arguments to Unsafe.setMemory() on PPC when offheap is enabled#7145
0xdaryl merged 5 commits into
eclipse-omr:masterfrom
midronij:offheap-unsafe-setmemory

Conversation

@midronij

@midronij midronij commented Oct 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Add runtime array check to generated assembly code for Unsafe.setMemory() on PPC and, when offheap/balanced GC policy is in use, generate assembly code to perform an arrayCHK and, if the object is an array, adjust the arguments as follows:

In addition, as a performance improvement for both the offheap and gencon cases on P8 and up, use vector instructions (stxvd2x and, on P10 only, stxvl) to reduce loop iterations when setting bytes in memory to specified value.

Depends on: eclipse-openj9/openj9#18288

@midronij midronij mentioned this pull request Oct 16, 2023
Merged
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 08e3c4a to 259e57f Compare October 16, 2023 19:17
@midronij

midronij commented Oct 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@zl-wang @VermaSh here is the OMR side of the fix for Unsafe.setmemory() on PPC. Please feel free to review when you have a moment.

zl-wang
zl-wang suggested changes Oct 23, 2023
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch 3 times, most recently from 64105ac to b839c7f Compare November 8, 2023 19:52
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch 2 times, most recently from 9287df5 to 89f16f3 Compare May 29, 2024 14:37
@midronij

midronij commented May 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@zl-wang omr changes for Unsafe.setmemory are done! When you have a chance, could you please review and (if everything looks good) merge?

Just as a note: I mentioned this above as well, but this PR is dependent on this one (eclipse-openj9/openj9#18288), so they need to be merged together

@midronij midronij changed the title WIP: Adjust arguments to Unsafe.setMemory() on PPC when offheap is enabled Adjust arguments to Unsafe.setMemory() on PPC when offheap is enabled May 30, 2024
zl-wang
zl-wang suggested changes May 31, 2024
View reviewed changes

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high-level (applicable to whole implementation):

  1. watch out for coding style and indentation requirement;
  2. you need to spell out those registers cannot be gr0 (otherwise, you can run into unexpected results). Just look for code examples of other fast-path implementations.

Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@zl-wang

zl-wang commented May 31, 2024

Copy link
Copy Markdown
Contributor

there are a lot you need to re-do. i suspected stxvl semantic was not understood completely before implementation started.

zl-wang
zl-wang suggested changes May 31, 2024
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 89f16f3 to 3fbc461 Compare July 12, 2024 15:20
zl-wang
zl-wang suggested changes Jul 12, 2024
View reviewed changes

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let me know when you made substantial progress on this re-work.

Comment thread compiler/p/codegen/OMRTreeEvaluator.hpp Outdated
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 3fbc461 to d3335f6 Compare July 25, 2024 17:22
@midronij
Offheap Adjustments for Unsafe.setMemory()
a4f6432 
When Unsafe.setMemory() is called on an array and offheap changes are
enabled, adjust arguments so that dataAddr is passed in as base address
of object.

Signed-off-by: midronij <jackie.midroni@ibm.com>
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch 2 times, most recently from 5a082aa to b0ebe22 Compare August 12, 2024 03:45
zl-wang
zl-wang suggested changes Aug 13, 2024
View reviewed changes

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i will come back to finish the review tmr

Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
zl-wang
zl-wang suggested changes Aug 13, 2024
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch 2 times, most recently from ba5f9a2 to 5455f66 Compare August 15, 2024 20:41
zl-wang
zl-wang suggested changes Aug 16, 2024
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
@zl-wang

zl-wang commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

you seemed adding local variables as you go. you might have a high-level frame-work in your mind, but it is not exhibited clearly. that makes reading your code harder. i need to jump up and down the code to piece them together ... making senses of the consistency of various conditions. strongly recommend you to program in a style easier to follow.

@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 5455f66 to 2b9817b Compare August 16, 2024 18:49
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 2b9817b to a0700bc Compare August 16, 2024 18:54
zl-wang
zl-wang suggested changes Aug 16, 2024
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch 2 times, most recently from fc7a0f0 to 48db45b Compare August 20, 2024 20:21
@midronij

midronij commented Aug 20, 2024

Copy link
Copy Markdown
Contributor Author

@zl-wang I've addressed your most recent round of review comments. If everything looks good, then I believe we can move on to CI testing

zl-wang
zl-wang suggested changes Aug 21, 2024
View reviewed changes

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

simple changes i expected ...

Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 48db45b to bf08bc6 Compare August 21, 2024 16:05
zl-wang
zl-wang suggested changes Aug 21, 2024
View reviewed changes
Comment thread compiler/p/codegen/OMRTreeEvaluator.cpp Outdated
@midronij
Skip uneccesary runtime array check for Unsafe.setMemory()
3107226 
Only generate runtime array check in setmemoryEvaluator() if it is
needed (i.e.: object type is unknown at compile time).

Signed-off-by: midronij <jackie.midroni@ibm.com>
@midronij
Add NULLCHK on object address passed in to Unsafe.setMemory()
0387944 
In situations where an array check is needed, there are scenarios in
which we do not want to modify the dest base address:

1.) If the object is a NULL reference (since we can't load dataAddr
    from a NULL pointer)
2.) If the object is a non-array object

Thus, before the array check is performed, a null test is needed to
account for situation (1).

Signed-off-by: midronij <jackie.midroni@ibm.com>
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from bf08bc6 to d51cac0 Compare August 21, 2024 16:46
zl-wang
zl-wang approved these changes Aug 21, 2024
View reviewed changes

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks reasonable now ...

@zl-wang

zl-wang commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

@vijaysun-omr this needs a coordinated merge together with eclipse-openj9/openj9#18288

@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from d51cac0 to 2d93e3e Compare August 22, 2024 14:41
@dsouzai

dsouzai commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

jenkins build all

@dsouzai

dsouzai commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

jenkins build aix,plinux

@midronij
Avoid allocating extra register when Unsafe.setMemory() offset is con…
cf8b0b5 
…stant

When destOffset is a constant 16-bit value, it can be represented as the
immediate value argument to addi when calculating the final destination
address (i.e.: dest = base address + offset). This allows us to allocate
one less register when generating the assembly code sequence for
Unsafe.setMemory().

Signed-off-by: midronij <jackie.midroni@ibm.com>
@midronij
Use Vector Instructions to Optimize Unsafe.setMemory() on PPC
e714da1 
On P8 and higher, we can make use of vector stores (stxvd2x and, for P10
specifically, stxvl) to reduce the number of memory accesses and avoid
checks needed to set residual bytes in the assembly code that is generated
for Unsafe.setMemory().

Signed-off-by: midronij <jackie.midroni@ibm.com>
@midronij
midronij force-pushed the offheap-unsafe-setmemory branch from 2d93e3e to e714da1 Compare August 23, 2024 14:49
@0xdaryl 0xdaryl self-assigned this Aug 28, 2024
@0xdaryl

0xdaryl commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

jenkins build aix,plinux

@0xdaryl
0xdaryl merged commit 6dc282e into eclipse-omr:master Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes
@aviansie-ben aviansie-ben Awaiting requested review from aviansie-ben
+1 more reviewer
@zl-wang zl-wang zl-wang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@0xdaryl 0xdaryl

Labels

arch:power comp:compiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@midronij @zl-wang @dsouzai @0xdaryl