-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8292153: x86: Represent Registers as values #9815
Conversation
👋 Welcome back vlivanov! A progress list of the required criteria for merging this PR into |
Webrevs
|
const Register empty = 0; // will never be used, in order not | ||
// to change a signature for crc32c_IPL_Alg2_Alt2 | ||
// between 64/32 I'm just keeping it here | ||
const Register empty = noreg; // will never be used, in order not |
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.
Shade and I were looking at this bug a week or so ago, but agreed to leave it because this patch would find and fix it. Good!
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 good to me.
@iwanowww 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 22 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 |
Thanks for the reviews, Vladimir and Andrew. /integrate |
Going to push as commit 755ecf6.
Your commit was automatically rebased without conflicts. |
As of now, Registers of all sorts use pointer-based representation. It's error-prone, because compilers perform implicit conversions between pointers and integral values. Also, it constraints
MacroAssembler
API where it may introduce ambiguity between overloads.Proposed change hides pointer representation behind value class.
Pointer-based representation is kept to avoid massive migration of users (from
->
to.
) and to enable incremental migration on per-platform basis (pointer-based representation is assumed in shared code).Code quality doesn't suffer (and even slightly improves):
https://godbolt.org/z/d8dGM1eY1
(I noticed one downside: slowdebug builds become slower, because
operator->
isn't inlined there. If it becomes a problem, migrating performance-sensitive places from->
to.
should solve the problem.)Testing: hs-tier1 - hs-tier5
PS: a number of cleanups are incorporated. In particular, I decided to expand all macros from
register.hpp
because IMO they add confusion rather than improve readability.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9815/head:pull/9815
$ git checkout pull/9815
Update a local copy of the PR:
$ git checkout pull/9815
$ git pull https://git.openjdk.org/jdk pull/9815/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9815
View PR using the GUI difftool:
$ git pr show -t 9815
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9815.diff