RFR: 8230203: Replace markWord enums with uintptr_t constants

[Stefan Karlsson]

On 2019-08-27 16:37, Doerr, Martin wrote:
> Hi Stefan,
>
>>> I think it would be better to use C++ 2011 Strongly-typed Enums.
>> Could you elaborate why it would be better to use them in this case?
> IMHO enums are better to read and edit. I don't like "static const uintptr_t" replicated 100 times.

OK. Point taken.

> In addition, static consts usually spam the binaries with stuff which is never needed while enums are only for the compiler.

I thought that wasn't a problem unless you, for example, tried to 
take/use the address of the constant.

Are you so opposed to this change that you think we should not do it? 
I'd also like to hear feedback regarding this from other reviewers.

Thanks,
StefanK
>
> I don't understand what keeps us from targeting JEP 347 to jdk14.
>
> Best regards,
> Martin
>
>
>> -----Original Message-----
>> From: Stefan Karlsson <stefan.karlsson at oracle.com>
>> Sent: Dienstag, 27. August 2019 13:08
>> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
>> <david.holmes at oracle.com>; hotspot-dev <hotspot-
>> dev at openjdk.java.net>; Kim Barrett <kim.barrett at oracle.com>
>> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
>> constants
>>
>> Hi Martin,
>>
>> On 2019-08-27 10:22, Doerr, Martin wrote:
>>> Hi,
>>>
>>> I think it would be better to use C++ 2011 Strongly-typed Enums.
>> Could you elaborate why it would be better to use them in this case?
>>
>> AFAICT these enums where only enums because that used to be the way
>> constants were written in HotSpot. There are some comments around this in:
>> https://bugs.openjdk.java.net/browse/JDK-8153116
>>
>>    Can we still not use them?
>>> AFAIK all compilers which are used for jdk/jdk support that.
>> I don't think we can go there until we get:
>> https://openjdk.java.net/jeps/347 - JEP 347: Adopt C++14 Language
>> Features in HotSpot
>>
>> Thanks,
>> StefanK
>>
>>> Best regards,
>>> Martin
>>>
>>>
>>>> -----Original Message-----
>>>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf
>> Of
>>>> David Holmes
>>>> Sent: Dienstag, 27. August 2019 10:02
>>>> To: Stefan Karlsson <stefan.karlsson at oracle.com>; hotspot-dev
>> <hotspot-
>>>> dev at openjdk.java.net>
>>>> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
>>>> constants
>>>>
>>>> Hi Stefan,
>>>>
>>>> On 27/08/2019 5:32 pm, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch to convert the enum constants in markWord
>> into
>>>>> static const unintptr_t constants.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8230203/webrev.01/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8230203
>>>> Overall this seems like a good cleanup, but the introduced "int" casts
>>>> now jump out at me. In:
>>>>
>>>>     src/hotspot/cpu/aarch64/aarch64.ad
>>>>     src/hotspot/cpu/arm/macroAssembler_arm.cpp
>>>>     src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
>>>>
>>>> it would seem to me that all the "add -(int)markWord::monitor_value"
>>>> would be cleaner/clearer as: "sub markWord::monitor_value". But that is
>>>> a bit out of scope for your change - it just jumped out at me. I expect
>>>> add is more efficient than sub.
>>>>
>>>> In  src/hotspot/share/opto/macro.cpp I don't understand why this int
>>>> cast is needed:
>>>>
>>>>          Node* not_biased_ctrl =  opt_bits_test(ctrl, region, 3, x_node,
>>>> !
>>>> (~(int)markWord::age_mask_in_place), 0);
>>>>
>>>> I'm unclear if this is really
>>>>
>>>> ~(int)markWord::age_mask_in_place
>>>>
>>>> or
>>>>
>>>> (int)(~markWord::age_mask_in_place)
>>>>
>>>> ? If the latter then okay. If the former then I don't grok it. :)
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> This is a follow-up RFE from the review of JDK-8229258, where a number
>>>>> of unnecessary casts were pointed out.
>>>>>
>>>>> There was also a recent bug in this area, JDK-8221725, where Thomas
>>>>> points out the problem of relying on the bit width of the enum values.
>>>>>
>>>>> This patch:
>>>>> 1) changes all constants to have the same type as the markWord::_value
>>>>> field.
>>>>> 2) Removes unnecessary uintptr_t casts
>>>>> 3) Removes uintx casts, because they are typedefed to be uintptr_t
>>>>> 4) Adds explicit casts to (int) when values are negated.
>>>>> 5) Try to only use uintptr_t inside markWord.
>>>>> 6) Leaves the use of intptr_t for parameters and return types of the
>>>>> different hash functions.
>>>>>
>>>>> Tested with tier1-3
>>>>>
>>>>> Thanks,
>>>>> StefanK