RFR: 8230203: Replace markWord enums with uintptr_t constants

[David Holmes]

On 27/08/2019 10:52 pm, Stefan Karlsson wrote:
> 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.

Okay - this was really just an aside comment.

Thanks,
David
-----

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