RFR: 8230203: Replace markWord enums with uintptr_t constants

[Stefan Karlsson]

On 2019-08-27 10:01, David Holmes wrote:
> 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.
> 

I agree.

> 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. :)

Without casting we get the following error message:
src/hotspot/share/opto/macro.cpp:2229:40: error: overflow in conversion 
from 'uintptr_t' {aka 'long unsigned int'} to 'int' changes value from 
'18446744073709551495' to '-121' [-Werror=overflow]
                                        (~markWord::age_mask_in_place), 0);

The parameter type for opt_bits_test is int. So, I used 
'~(int)markWord::age_mask_in_place' to solve it. The intention was to 
'take the small positive 64 bit wide integer, make it into a positive 32 
bit wide integer, and then flip the bits'. It felt more intuitive 
(safer?) to narrow a positive integer than to cast a large positive 
integer to a smaller negative integer.

Does that make sense, or did I miss something? :)

Thanks,
StefanK

> 
> 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