RFR: 8230203: Replace markWord enums with uintptr_t constants

[David Holmes]

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

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