RFR: 8363943: ARM32: Represent Registers as values [v2]

[Ivan]

On Mon, 10 Nov 2025 11:17:15 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Ivan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Proposed review changes were applied
>
> src/hotspot/cpu/arm/register_arm.hpp line 152:
> 
>> 150: constexpr Register R9 = as_Register(9);
>> 151: constexpr Register R10 = as_Register(10);
>> 152: constexpr Register R11 = as_Register(11);
> 
> Indent these like:
> 
> 
> constexpr Register R8  = as_Register( 8);
> constexpr Register R9  = as_Register( 9);
> constexpr Register R10 = as_Register(10);
> constexpr Register R11 = as_Register(11);

Applied in `962b01b602c0f42b95f8a3ad4f58d84b17db3c6f` commit

> src/hotspot/cpu/arm/register_arm.hpp line 264:
> 
>> 262: constexpr FloatRegister S4_reg = as_FloatRegister(4);
>> 263: constexpr FloatRegister S5_reg = as_FloatRegister(5);
>> 264: constexpr FloatRegister S6_reg = as_FloatRegister(6);
> 
> Take a chance on renaming these `S${X}_reg` to just `S${X}`? I spot-checked their usages, and there are only a few places that need adjustments.

At the top of these definitions there is a comment

/*
 * S1-S6 are named with "_reg" suffix to avoid conflict with
 * constants defined in sharedRuntimeTrig.cpp
 */
 ```
 And the definitions from `sharedRuntimeTrig.cpp` are still there
 ```
 static const double
S1  = -1.66666666666666324348e-01, /* 0xBFC55555, 0x55555549 */
S2  =  8.33333333332248946124e-03, /* 0x3F811111, 0x1110F8A6 */
S3  = -1.98412698298579493134e-04, /* 0xBF2A01A0, 0x19C161D5 */
S4  =  2.75573137070700676789e-06, /* 0x3EC71DE3, 0x57B1FE7D */
S5  = -2.50507602534068634195e-08, /* 0xBE5AE5E6, 0x8A2B9CEB */
S6  =  1.58969099521155010221e-10; /* 0x3DE5D93A, 0x5ACFD57C */
 ```
 
 Should I ignore it and change `S${X}_reg` to `S${X}`? Or I shall avoid the conflict some other way?

> src/hotspot/cpu/arm/register_arm.hpp line 265:
> 
>> 263: constexpr FloatRegister S5_reg = as_FloatRegister(5);
>> 264: constexpr FloatRegister S6_reg = as_FloatRegister(6);
>> 265: constexpr FloatRegister S7 = as_FloatRegister(7);
> 
> Also, indent this like:
> 
> 
> constexpr FloatRegister S8  = as_FloatRegister( 8);
> constexpr FloatRegister S9  = as_FloatRegister( 9);
> constexpr FloatRegister S10 = as_FloatRegister(10);
> constexpr FloatRegister S11 = as_FloatRegister(11);

Applied in `962b01b602c0f42b95f8a3ad4f58d84b17db3c6f` commit

> src/hotspot/cpu/arm/register_arm.hpp line 427:
> 
>> 425: constexpr VFPSystemRegister FPSCR = as_VFPSystemRegister(  1);
>> 426: constexpr VFPSystemRegister MVFR0 = as_VFPSystemRegister(0x6);
>> 427: constexpr VFPSystemRegister MVFR1 = as_VFPSystemRegister(0x7);
> 
> You can use `VFPSystemRegister` enum values as arguments here, correct? Like:
> 
> 
> constexpr VFPSystemRegister MVFR1 = as_VFPSystemRegister(VFPSystemRegister::MVFR1);

Yes, that looks much more clear, applied in `962b01b602c0f42b95f8a3ad4f58d84b17db3c6f` commit

> src/hotspot/cpu/arm/vmreg_arm.hpp line 52:
> 
>> 50:       return (value() % Register::max_slots_per_register == 0);
>> 51:     } else if (is_FloatRegister()) {
>> 52:       return true; // Single slot
> 
> I guess. But for safety, we can still do `% FloatRegister::max_slot_per_register == 0`, just in case we ever need to adjust it?

Sure, sounds reasonable, applied in `962b01b602c0f42b95f8a3ad4f58d84b17db3c6f` commit

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26525#discussion_r2516040914
PR Review Comment: https://git.openjdk.org/jdk/pull/26525#discussion_r2516039154
PR Review Comment: https://git.openjdk.org/jdk/pull/26525#discussion_r2516039986
PR Review Comment: https://git.openjdk.org/jdk/pull/26525#discussion_r2516048503
PR Review Comment: https://git.openjdk.org/jdk/pull/26525#discussion_r2516045497