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
8260369: [PPC64] Add support for JDK-8200555 #2358
Conversation
|
@TheRealMDoerr The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me.
Please review and act on my inline comment on interp_masm_ppc_64.cpp(line 507).
Thanks, Lutz
load_heap_oop(result, arrayOopDesc::base_offset_in_bytes(T_OBJECT), result, | ||
tmp1, tmp2, | ||
MacroAssembler::PRESERVATION_FRAME_LR, | ||
MacroAssembler::MacroAssembler::PRESERVATION_NONE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need this duplicate class specification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@TheRealMDoerr This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 63 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good! Took some time to understand why load_resolved_reference_at_index
can be used without the need for register preservation, but seems to work just fine! I just have some very minor styling complains. Would be great if you could address those, but that's definitely no requirement.
Rconst_method = R8_ARG6; | ||
Rconst_method = R8_ARG6, | ||
Rconst_pool = R9_ARG7, | ||
R10_tmp = R10_ARG8; | ||
|
||
assert_different_registers(Rsize_of_parameters, Rsize_of_locals, parent_frame_resize, top_frame_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider strengthening the assertion. Rconst_method
, for instance, must not be equals to Rsize_of_parameters
.
Register parent_frame_resize = R6_ARG4, // Frame will grow by this number of bytes. | ||
top_frame_size = R7_ARG5, | ||
Rconst_method = R8_ARG6; | ||
Rconst_method = R8_ARG6, | ||
Rconst_pool = R9_ARG7, | ||
R10_tmp = R10_ARG8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three variable naming styles in 5 LOCs. Would be great to have a consistent naming style throughout this function.
__ bind(Ldone); | ||
} | ||
|
||
// Load receiver if needed (after appendix is pushed so parameter size is correct). | ||
if (load_receiver) { | ||
const Register Rparam_count = Rscratch; | ||
const Register Rparam_count = Rscratch1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other variables of type Register declared in this method aren't const
, although they aren't subject to change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like GitHub didn't show the latest changes to me. Looks good as well, nice optimization!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing the feeback. I have nothing to add!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me.
Thanks for the reviews! |
@TheRealMDoerr Since your change was applied there have been 63 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 48f5220. |
I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
I have to change register usage. That's what makes this change a bit larger.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2358/head:pull/2358
$ git checkout pull/2358