Skip to content

8352690: [JVMCI] Add the set of caller saved registers to j.i.v.c.CallingConvention #24185

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
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

8352690: [JVMCI] Add the set of caller saved registers to j.i.v.c.CallingConvention #24185

wants to merge 3 commits into from

Conversation

kittyoracle
Copy link

@kittyoracle kittyoracle commented Mar 24, 2025 •
edited by openjdk bot
Loading

Hi,

This patch adds the set of caller-saved registers to j.i.v.c.CallingConvention as well as refactor the array of argument locations into a List.

Please take a look and share your thoughts, thanks a lot.

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Error

 ⚠️ OCA signatory status must be verified

Issue

  • JDK-8352690: [JVMCI] Add the set of caller saved registers to j.i.v.c.CallingConvention (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24185/head:pull/24185
$ git checkout pull/24185

Update a local copy of the PR:
$ git checkout pull/24185
$ git pull https://git.openjdk.org/jdk.git pull/24185/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24185

View PR using the GUI difftool:
$ git pr show -t 24185

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24185.diff

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Mar 24, 2025
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 2025

Hi @kittyoracle, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user kittyoracle" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

openjdk bot commented Mar 24, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Mar 24, 2025

@kittyoracle The following labels will be automatically applied to this pull request:

  • graal
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Mar 24, 2025
@openjdk
Copy link

openjdk bot commented Mar 24, 2025

@kittyoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@kittyoracle
Copy link
Author

/covered

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Mar 24, 2025
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 2025

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

dougxc
dougxc reviewed Mar 24, 2025
View reviewed changes
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/CallingConvention.java
@@ -60,10 +71,11 @@ public interface Type {
* call
* @param argumentLocations the ordered locations in which the arguments are placed
*/
public CallingConvention(int stackSize, AllocatableValue returnLocation, AllocatableValue... argumentLocations) {
public CallingConvention(int stackSize, AllocatableValue returnLocation, List<? extends AllocatableValue> argumentLocations, Collection<? extends AllocatableValue> callerSavedLocations) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice API improvement (array -> list for the type of argumentLocations) but will need Graal adaption.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Graal adaption will likely fail the JDK21 compilation. Make sure to run a JDK21 gate

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/CallingConvention.java Outdated
@@ -94,18 +106,21 @@ public int getStackSize() {
* Gets the number of locations required for the arguments.
*/
public int getArgumentCount() {
return argumentLocations.length;
return argumentLocations.size();
}

/**
* Gets the locations required for the arguments.
Copy link
Member

Choose a reason for hiding this comment

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

update javadoc to state that the returned list is unmodifiable

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/CallingConvention.java Outdated
}

/**
* Get the list of call-clobbered (caller-saved) registers.
Copy link
Member

Choose a reason for hiding this comment

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

update javadoc to state that the returned set is unmodifiable

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 21, 2025

@kittyoracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk openjdk bot changed the title 8352690: [jvmci] Add the set of caller saved registers to j.i.v.c.CallingConvention 8352690: [JVMCI] Add the set of caller saved registers to j.i.v.c.CallingConvention Apr 21, 2025
dougxc
dougxc approved these changes Apr 21, 2025
View reviewed changes
Copy link
Member

@dougxc dougxc left a comment

Choose a reason for hiding this comment

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

Re-approved after test fixes. I also uppercased "JVMCI" in JDK-8352690 (for consistency) so you'll need to do the same for this PR's title.

@kittyoracle kittyoracle marked this pull request as draft April 22, 2025 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mur47x111 mur47x111 mur47x111 left review comments

@dougxc dougxc dougxc approved these changes

Assignees
No one assigned
Labels
graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kittyoracle @mur47x111 @dougxc