-
Notifications
You must be signed in to change notification settings - Fork 114
8349110: [lworld] Intrinsics for Unsafe.get/putFlatValue #1482
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
Conversation
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
@merykitty 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 14 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. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
Thanks for working on this Quan-Anh! I executed some quick testing and noticed that
It mostly seems to fail on linux-aarch64-debug in various configurations for some reason but also once on x64 with |
I also noticed that there are still two leftover ToDos from JDK-8239003 in TestIntrinsics. I think |
@TobiHartmann It was because I need to buffer the |
Thanks for investigating! What about this one? valhalla/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java Lines 72 to 73 in 0bbfece
|
But that should only be needed if |
Ah yes I missed that, removed!
Unfortunately |
Ah, makes sense! Thanks. I'll re-run testing. |
Testing looks good now except for one last failure with
|
@TobiHartmann The crash is due to JDK-8322547. I have excluded the test. |
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.
Ah, right. Let's convert this one to a bug then. Your changes look good to me. Thanks again!
@TobiHartmann Thanks a lot for your reviews! |
Going to push as commit d1c3062.
Your commit was automatically rebased without conflicts. |
@merykitty Pushed as commit d1c3062. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
This PR implements intrinsics for
Unsafe::get/putFlatValue
and enables the corresponding tests inTestIntrinsics
.Please kindly review, thanks very much.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1482/head:pull/1482
$ git checkout pull/1482
Update a local copy of the PR:
$ git checkout pull/1482
$ git pull https://git.openjdk.org/valhalla.git pull/1482/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1482
View PR using the GUI difftool:
$ git pr show -t 1482
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1482.diff
Using Webrev
Link to Webrev Comment