RFR: 8230203: Replace markWord enums with uintptr_t constants

[Stefan Karlsson]

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