-
Notifications
You must be signed in to change notification settings - Fork 17
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
8317352: [Lilliput] Sync with upstreaming PRs #110
Conversation
👋 Welcome back rkennke! A progress list of the required criteria for merging this PR into |
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.
This looks okay, but I have a question that I think I asked before:
src/hotspot/cpu/x86/x86_64.ad
Outdated
%{ | ||
predicate(UseCompactObjectHeaders); | ||
match(Set dst (LoadNKlass mem)); | ||
effect(KILL cr); | ||
ins_cost(125); // XXX | ||
format %{ "movl $dst, $mem\t# compressed klass ptr" %} | ||
ins_encode %{ | ||
assert($mem$$disp == oopDesc::klass_offset_in_bytes(), "expect correct offset 4, but got: %d", $mem$$disp); | ||
assert($mem$$disp == TypeOopPtr::klass_offset_in_bytes(), "expect correct offset 4, but got: %d", $mem$$disp); |
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.
Haven't we decided to avoid changing oopDesc::klass_offset_in_bytes
to TypeOopPtr::klass_offset_in_bytes
? I remember this discussion, but cannot find the reference it it right now.
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.
Right. That was in Lilliput 21 (and/or 17). Shall we also get rid of the extra assert that the change provides us here? And also in the upstreaming PR openjdk/jdk#13961?
@rkennke 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:
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Thanks! /integrate |
Going to push as commit 39f08e7. |
This PR syncs the Lilliput mainline repo with the changes that we are proposing in the upstreaming PRs. We have accumulated a number of diversions. Ideally, the Lilliput repo should mirror what is proposed for upstreaming, except where it integrates the upstreaming PRs in various places.
Notable changes:
There is a remaining problem with CDS (the changes in archiveBuilder.cpp) which affect only Windows, which I am sorting out separately.
Testing:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/lilliput.git pull/110/head:pull/110
$ git checkout pull/110
Update a local copy of the PR:
$ git checkout pull/110
$ git pull https://git.openjdk.org/lilliput.git pull/110/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 110
View PR using the GUI difftool:
$ git pr show -t 110
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/lilliput/pull/110.diff
Webrev
Link to Webrev Comment