RFR: 8230203: Replace markWord enums with uintptr_t constants

[Stefan Karlsson]

On 2019-08-27 13:37, David Holmes wrote:
> On 27/08/2019 9:21 pm, Stefan Karlsson wrote:
>> 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? :)
> 
> I understand the error message but I'm a little less clear about the 
> overall change. Previously the enums were int types and so only 32-bit 
> always.

This is not entirely true. They could be smaller than 32-bit, and could 
have been larger than 32-bit if high-order bits were used. But yes, 
previously these values were not larger than 32-bit.

  But the 32-bit masks were applied to 64-bit values on 64-bit
> systems, so presumably integer promotion came into play in those cases 
> as there were no explicit casts. Now the constants are 64-bit on 64-bit 
> and 32-bit on 32-bit and they match the size of the field being masked, 
> so that is good. But it then leads me to wonder why we are passing a 
> 64-bit mask into opt_bits_test when it can only deal with a 32-bit mask? 
> Makes me wonder whether something in there isn't also the wrong type?

Maybe someone from the compiler team can comment on this?

For this specific case we know that the bits will fit inside a 32-bit 
sized integer and exploit that fact.

We do similar narrowing inside the markWord class. For example:

   uint     age()           const { return mask_bits(value() >> 
age_shift, age_mask); }

Thanks,
StefanK

> 
> Thanks,
> David
> 
>>
>> 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