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

JDK-8269408: [lworld] [lqagain] Withfield and field resolution update #465

Closed
wants to merge 3 commits into from

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented Jun 25, 2021

Please review those changes in the interpreter and the field resolution code.

With the L/Q model, field resolution must now check that putfield is applied only on identity objects and withfield is applied only on primitive objects.
The interpreter now performs the required null check on the receiver when executing a withfield bytecode.
The implementation of withfield has been reworked to remove some of the costly operations it was using (retrieving the last Java frame to be able to extract arguments of the withfield bytecode). The old version of withfield in the interpreter runtime has not been removed because it is still used by the aarch64 platform.
The withfield unit test has been extended to cover all kind of fields.
No additional test regarding field resolution has been added yet, I'll work with Harold to define and implement them. They'll be integrated in a later patch.

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8269408: [lworld] [lqagain] Withfield and field resolution update

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/465/head:pull/465
$ git checkout pull/465

Update a local copy of the PR:
$ git checkout pull/465
$ git pull https://git.openjdk.java.net/valhalla pull/465/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 465

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/465.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 25, 2021

👋 Welcome back fparain! A progress list of the required criteria for merging this PR into lqagain will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

@fparain This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269408: [lworld] [lqagain] Withfield and field resolution update

Reviewed-by: dsimms

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 1 new commit pushed to the lqagain branch:

  • a2c8822: 8267932: [lworld] JIT support for the L/Q model (step 2)

Please see this link for an up-to-date comparison between the source branch of this pull request and the lqagain branch.
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.

➡️ To integrate this PR with the above commit message to the lqagain branch, type /integrate in a new comment.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 25, 2021

Webrevs

Copy link
Member

@MrSimms MrSimms left a comment

Looks good, one comment, on comments, the description that is in the PR is quite useful, probably should appear in code somewhere:

The old version of withfield in the interpreter runtime has not been removed because it is still used by the aarch64 platform.

I.e. a comment on withfield vs withfield2

@fparain fparain closed this Jul 20, 2021
@fparain fparain deleted the withfield_int branch Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants