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

8297539: Consolidate the uses of the int<->float union conversion trick #12384

Closed
wants to merge 2 commits into from

Conversation

afshin-zafari
Copy link
Contributor

@afshin-zafari afshin-zafari commented Feb 2, 2023

Discussion

There are different ways of casting/converting integral types to floating point types and vice versa in the code.
These conversions are changed to invocation of a centralised cast<To>(From) method in primitiveConversions.hpp. To make this changes build and run, some new cast methods are introduced and header dependencies changed.

Patch

  • All instances of using union technique that are used for int<->float and long<->double type conversions are replaced by PrimitiveConversions::cast<To>(From) method.
    • Instances of, for example, converting int<->intptr_t are not changed.
  • After this change, JavaValue also uses the PrimitiveConversions::cast<To>(From) templates. Therefore, all the union conversions for int<-> float and long <-> double in hotspot are now centralised in one location.
  • Templates in PrimitiveConversions class are numbered from 1-9 and all lines of code that use PrimitivEConversions::cast<To>(From) have comments saying which templated cast<To>(From) matches. Issue number 8297539 is also in the comments to make the search/reviews more easier.
  • The new 'cast(From)' templates are numbered 6-9 for some instances where existing templates (1-5) did not match.
  • globalDefinitions.hpp now includes primitiveConversions.hpp.
  • primitiveConversions.hpp does NOT include globalDefinitions.hpp any more. Some typedef's are added to it to compensate the missing types.
  • The followings are not changed:
    • jni.hpp
    • ciConstant.hpp is not changed, since uinon is not used for converting int <->float or long <-> double. There are some asserts that allow setter/getter used only for the same types.
    • bytecodeInterpreter_zero.hpp is not changed. Writing to and reading from the VMJavaVal64 are always with the same type and no conversion is made.
    • union with bit fields are not changed.
    • json.hpp and json.cpp in src/hotspot/share/utilities.

Test

mach5 tiers 1-5 passed, tiers 6-8 in progress


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

Issue

  • JDK-8297539: Consolidate the uses of the int<->float union conversion trick

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12384

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 2, 2023

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 2, 2023
@openjdk
Copy link

openjdk bot commented Feb 2, 2023

@afshin-zafari The following label will be automatically applied to this pull request:

  • hotspot

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

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 2, 2023
@mlbridge
Copy link

mlbridge bot commented Feb 2, 2023

Webrevs

Copy link
Contributor

@rose00 rose00 left a comment

Choose a reason for hiding this comment

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

I think the term "cast" in this API means too many different things, including things that a C programmer would not think of as simple casting. To be clear, this is a problem I have with the existing API, not just this PR.

I request we start to make a clearer distinction between conversions which preserve values (perhaps with truncation or loss of precision) and reinterpretations which discard values but keep the underlying bits. Both may be called casts, but it is asking for bugs to use the name for both kinds of requests.

(Using the word cast for something that preserves at least some values is how the JLS handles conversion terminology, and the JLS makes reinterpret casts available only via special methods. I know C++ has its own lexicon of terms, but I claim some privilege on this project for Java terms as well.)

I know this work here is changing from names like "jint_cast" which perform reinterpretations, but I always viewed those names as suspicious, and had to learn by rote that they refer to reinterpretations and not true C (or Java) casts. Naming it just "cast" with a tricky type parameter is a move even farther in the wrong direction.

Having the function named "cast" in a class named PrimitiveConversions (not PrimitiveReinterpretations) makes it dangerously easy to miss that the functions in that API are (apparently?) all reinterpretations, and none are what a Java programmer would call a cast. I suggest either renaming of the class (PrimitiveReinterpretations) or better changing the name from "cast" to something like "reinterpret". Then casual readers of code will be less likely to make errors of… reinterpretation.

@jcking
Copy link
Contributor

jcking commented Feb 2, 2023

I think the term "cast" in this API means too many different things, including things that a C programmer would not think of as simple casting. To be clear, this is a problem I have with the existing API, not just this PR.

I request we start to make a clearer distinction between conversions which preserve values (perhaps with truncation or loss of precision) and reinterpretations which discard values but keep the underlying bits. Both may be called casts, but it is asking for bugs to use the name for both kinds of requests.

(Using the word cast for something that preserves at least some values is how the JLS handles conversion terminology, and the JLS makes reinterpret casts available only via special methods. I know C++ has its own lexicon of terms, but I claim some privilege on this project for Java terms as well.)

I know this work here is changing from names like "jint_cast" which perform reinterpretations, but I always viewed those names as suspicious, and had to learn by rote that they refer to reinterpretations and not true C (or Java) casts. Naming it just "cast" with a tricky type parameter is a move even farther in the wrong direction.

Having the function named "cast" in a class named PrimitiveConversions (not PrimitiveReinterpretations) makes it dangerously easy to miss that the functions in that API are (apparently?) all reinterpretations, and none are what a Java programmer would call a cast. I suggest either renaming of the class (PrimitiveReinterpretations) or better changing the name from "cast" to something like "reinterpret". Then casual readers of code will be less likely to make errors of… reinterpretation.

It should be noted that bit_cast is the chosen term for C++ starting in C++20, which reinterprets. Whether that is clear or not is a separate issue, but using cast is consistent at least even if odd. Maybe we just change it to Bits::cast?

@rose00
Copy link
Contributor

rose00 commented Feb 2, 2023

bit_cast is the chosen term for C++ starting in C++20, which reinterprets

Good. I would be very content with bit_cast, or some use of the older word "reinterpret". The problem with plain "cast" is the overloading of meanings. (And then there's the extra push in the wrong direction from the name "PrimitiveConversions".) We are not just C++ programmers here; many of us are Java programmers as well.

@afshin-zafari
Copy link
Contributor Author

bit_cast is the chosen term for C++ starting in C++20, which reinterprets

Good. I would be very content with bit_cast, or some use of the older word "reinterpret". The problem with plain "cast" is the overloading of meanings. (And then there's the extra push in the wrong direction from the name "PrimitiveConversions".) We are not just C++ programmers here; many of us are Java programmers as well.

I need this conversation completes to apply the required changes.

@afshin-zafari afshin-zafari requested review from kimbarrett, jcking and rose00 and removed request for kimbarrett, jcking and rose00 February 7, 2023 15:30
@kimbarrett
Copy link

I think there are some significant problems with the [v2] version (webrev 1). Afshin and I are going to meet, for better
bandwidth vs discussing in PR. Will summarize here later, but other folks might want to hold off looking at that version.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 15, 2023

@afshin-zafari 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!

@afshin-zafari
Copy link
Contributor Author

I am going to do some more limited fixes for this RFE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review
5 participants