RFR: 8297539: Consolidate the uses of the int<->float union conversion trick
Justin King
jcking at openjdk.org
Thu Feb 2 22:30:17 UTC 2023
On Thu, 2 Feb 2023 17:35:27 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> ### 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<To>(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
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 28:
>
>> 26: #define SHARE_UTILITIES_GLOBALDEFINITIONS_HPP
>> 27:
>> 28: #include "metaprogramming/primitiveConversions.hpp"
>
> I think this should not be done, for the same reasons as not including the suggested bit_cast
> (https://git.openjdk.org/jdk/pull/11865). There is substantial overlap between this proposed
> change and that one, though I think the bit_cast proposal is more problematic.
>
> Rather than adding a new dependency here, separate out the code that uses the conversions
> into new files and update the (much smaller number than use globalDefinitions.hpp) users
> of those functions to include the new headers.
I previously updated the bit_cast PR to just be `PrimitiveConversions::cast` implementation, but using __builtin_bit_cast when available. So this and that are basically the same.
-------------
PR: https://git.openjdk.org/jdk/pull/12384
More information about the hotspot-dev
mailing list