RFR: 8297539: Use PrimitiveConversions::cast for local uses of the int<->float union conversion trick [v2]

[Kim Barrett]

On Mon, 27 Mar 2023 14:32:22 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> **Only** the instances of using `union` for converting `int` to `float` are replaced with call to `PrimitiveConversions::cast<To>(From)` method. Some few cases with conversion of `long` <->`double` are also replaced with `PrimitiveConversions::cast<To>(From)`. The other instances where the union contains other types of fields than `int` and `float` are left unchanged.
>> 
>> ### Test
>> local hotspot:tier1
>> mach5 tiers 1-5
>
> Afshin Zafari has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - order of headers in includes.
>  - order of headers corrected

Overall this looks good.  Just a few minor nits and naming issues.

src/hotspot/cpu/aarch64/assembler_aarch64.cpp line 503:

> 501: 
> 502: static float unpack(unsigned value) {
> 503:   unsigned ival;

I'd like this to be initialized rather than assigned on the next line.

src/hotspot/cpu/arm/assembler_arm.hpp line 288:

> 286:     virtual bool f_lo_is_null() const { return (_num_bits & ((1 << 19) - 1)) == 0; }
> 287:     virtual int e() const { return ((_num_bits << 1) >> (23+1)) - 127; }
> 288:     virtual unsigned int s() const { return _num_bits >> 31; }

These should be override rather than virtual.  Similarly for double_num.

src/hotspot/cpu/arm/assembler_arm.hpp line 291:

> 289: 
> 290:    private:
> 291:     unsigned int _num_bits;

The name _num_bits is confusing; I spent a few moments trying to figure out what it was the number
of bits of.  I think just _bits or _raw_bits or something like that would be better.  Similarly in double_num.
I also think the class names would be better with "num" => "bits", but that likely has fannout, so maybe
later (if at all).

src/hotspot/cpu/arm/macroAssembler_arm.cpp line 677:

> 675: void MacroAssembler::mov_float(FloatRegister fd, jfloat c, AsmCondition cond) {
> 676:   Label skip_constant;
> 677:   jint accessor_i = PrimitiveConversions::cast<jint>(c);

I don't think the "accessor_" prefix has any meaning with this change, and just seems odd.
Just call the variable "bits" or "float_bits" or something like that?

src/hotspot/share/runtime/sharedRuntime.cpp line 242:

> 240:   // infinity operands.
> 241:   juint xbits_i = PrimitiveConversions::cast<juint>(x);
> 242:   juint ybits_i = PrimitiveConversions::cast<juint>(y);

The "_i" suffix here doesn't have any meaning anymore.  Just "xbits" and "ybits" would be better.
Similarly in below in "drem".

-------------

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13136#pullrequestreview-1359895552
PR Review Comment: https://git.openjdk.org/jdk/pull/13136#discussion_r1149807740
PR Review Comment: https://git.openjdk.org/jdk/pull/13136#discussion_r1149812015
PR Review Comment: https://git.openjdk.org/jdk/pull/13136#discussion_r1149813388
PR Review Comment: https://git.openjdk.org/jdk/pull/13136#discussion_r1149817150
PR Review Comment: https://git.openjdk.org/jdk/pull/13136#discussion_r1149818559